Conversation

edited

By locking the platform version we encourage a good practice which prevents people from messing up with a prod server that has a different version than the one on the dev's machine that ran composer up.

By committing the composer.lock file, we optimize a lot the installation process.
Here is a comparison of a create-project with/without the lock file:

Without lock

With lock

Nb of files downloaded

167

40

Peak Memory

274MB

120MB

Duration on regular network

19s

12s

The difference of duration can be much worse on slower networks.

Process-wise, this will require us to ship a new lock file with each new Symfony version. This feels like something we can automate in the releasing process, and will give us the opportunity to synchronize skeleton's version numbers with Symfony'.

This comment has been minimized.

While it's a very interesting approach, I don't like the idea of being locked into a specific version (say 7.0.8) if I'm running 7.1 or 7.2, it might lead to receiving "outdated" versions of the dependencies. I'm not exactly sure how else to do this neatly though. Would it help to specify the minimum patch versions in the requires instead? Perhaps a --super-optimized flag?

This comment has been minimized.

edited

This PR is for branch 3.4.
We will bump the platform to 7.1 on 4.0.
Then, of course, this file is just the starting point for projects and will be edited by users.
It's still useful to encourage people to actually use that setting.

This comment has been minimized.

I don't think this is a good idea. When I run create-project now, I will actually get wrong dependencies. Your composer.lock will give me dependencies for PHP 7.0.8 but I have 7.2.7 so I would get different library version combinations. If you commit that I have to run composer create-project, adjust the composer.json and then run composer update again. Nothing about this process helps to save memory, it just makes things more complicated.

I agree that configuring the PHP version in the platform config is a good idea to make sure it matches the production system but you have to do that in your local project not in the skeleton.

This comment has been minimized.

edited

@Toflar what matters is the time to first success. You will not have "wrong" dependencies, you will have dependencies that work on 7.2 also. And the skeleton is just the starting point, which we lock here to make it stable and fast to bootstrap.
I think that will improve the experience of installing Symfony a lot.

For developers less experienced with package management/composer or development in general, this might cause more problems than it solves in performance, as they will not receive the dependencies they'd expect.

This comment has been minimized.

edited

No, that's not true. Think about a dependency that defines requires like so:

"require": {
"php": ">7.0 <7.2",
}

because it uses something that explicitly only works for PHP 7.0.* and 7.1.*. I would get the dependency even though it fails on my system when I have PHP 7.2.*.
Sure, this example is probably very rare but that's what constraints are about. Any combination is possible.

you will have dependencies that work on 7.2 also

This is an assumption and you don't know if that's true. Using 5.6 and 7.* would have probably served as a better example. Also, you would need to update your composer.lock all the time when any of the dependencies releases a new - say - security relevant version. Otherwise this skeleton would provide a set of packages that contains packages with security issues.

It's not a good idea to commit the composer.lock for a skeleton. It will make it faster to bootstrap, yes but at the cost of probably providing a combination that doesn't work for your system. This approach will very likely cause issues at some point.

Regarding the platform config. I think it would be better if Flex checked the composer.json and showed a warning/recommendation to add the PHP version there to ensure you're not installing dependencies that do not match the target platform.

This comment has been minimized.

That's a skeleton. Dependencies are not random but minimal. This issue doesn't exist with any deps of the skeleton. About platform, putting a recommendation in Flex will add complexity for something that is unrelated to its business. Then, if people don't know how to deal with deps, its even more important to lock the platform, because they will just mess up with local vs remove versions of PHP.

This comment has been minimized.

For newcomers that means it all installs faster now but it probably installs packages with security issues or incompatibilities.

For experienced devs it means you have to run create-project, adjust the composer.json to match your platform and then run update.

Don't get me wrong, I love your efforts on trying to make this experience better but what I think it actually does is that it prefers performance over integrity and that's not a good decision. But that's my 2c, let's see what other devs think 😄

This comment has been minimized.

That's a skeleton. You WILL have to use composer very soon. Nothing new here.

Yeah but what's the point then? I will have to use composer update very soon anyway. So either it takes 19s and uses up 300MB of memory during create-project or it takes 19s and uses up 300MB of memory 5 minutes later when I have to run composer update😄

This comment has been minimized.

edited

@iltar I'm not 100% sure to understood your question, but if you're wondering how performance would change by narrowing the version windows (^3.4.12), the answer is not at all: solving the dependencies is not the resource consuming part. What is resource intensive is 1. downloading the json repositories from packagist 2. keeping them in memory. And this has to be done whatever the version windows are.

This comment has been minimized.

I'm mixed on this: on one side locking the platform prevents installing e.g. Doctrine DBAL 2.7 as @aschempp noticed, on the other side removing config.platform feels like a missed opportunity to set the option and encourage a best practice that prevents some nasty mistakes (we don't want to make Flex interactive so we don't want to go down with @anyt proposals.)

Still, OK to remove config.platform on my side.

This comment has been minimized.

By the way, I forgot to say that I like this a lot!! I agree with Nico that the first installation time is a critical metric. A minor question related to this: should we remove ext-ctype and ext-iconv from composer.json and replace them by Symfony's related polyfills?

PS: I also hope that some day the entire PHP community organizes a joint effort to massively improve Composer's performance. It's sad that Yarn takes a few seconds to do anything and Composer takes so long even for small apps. Of course I'm not complaining about Composer's maintainers. Thanks to them for everything they do 🙏

This comment has been minimized.

First of all, making things faster is a laudable goal. I like that a lot. Now, how to achieve that (without re-introducing an installer)?

Several issues raised in the PR.

The first issue is about deps that are not part of "symfony/symfony". They might be updated between two releases, and the lock won't be updated (that was also the case with the Standard Edition). This issue does not really exist for the bare skeleton (this repo) as we only depend on a sub-part of symfony/symfony (+ the polyfill), but it definitely exists for the website skeleton (which depends on Twig, Swiftmailer, Doctrine, ...).

The second issue is if you would have had more recent/different dependencies because your version of PHP is different locally (we compute the lock with 7.0.8, but you have PHP 7.2.4). I think this is marginal and not something we want to take into account. We can reconsider if that's really a problem later on. What's important is to not have the platform hardcoded in the composer.json file.

With that in mind, I'd like to propose something different from this PR:

We don't change the composer.json file and we only commit the composer.lock file (and we make sure that the right version of PHP is used to generate it like 7.0.8 for Symfony 3.4).

We setup a cron that each hour tries to update the composer.lock. If there are some changes, it commits them and tag a new version (for Symfony 4.1 for instance, the next version would be 4.1.13.1, and if a third-party dep is updated before 4.1.14, we would release 4.1.13.2).

By using the cron feature of Travis (where we can nicely get all the PHP versions we need), that should be completely automated.

That way, people creating a project will always have the latest versions of all deps, including transitive ones. And a simple composer update gives you whatever updates depending on your local PHP version. Huge gains without too many drawbacks

This comment has been minimized.

It's sad that Yarn takes a few seconds to do anything and Composer takes so long even for small apps.

This is comparing apples and oranges. PHP needs a SAT solver because you can use one class only ever once. Javascript doesn't because you can include the same modules in multiple versions if you work with modules 😄 Optimizing it in the current state is really challenging, I've tried for many, many hours...but this is a discussion for the composer/composer repository. I just wanted to make sure we do not compare PHP's package manager with npm or yarn in this issue 😊

This comment has been minimized.

A minor question related to this: should we remove ext-ctype and ext-iconv from composer.json and replace them by Symfony's related polyfills?

I didn't answer this: the answer is no. ctype and iconv are always enabled, except on maybe on Alpine & custom builds. We really want ppl to have a great experience from start, so that even ppl that do custom things have great perf.

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.