silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
722 stars 821 forks source link

RFC: Move to Symfony 4.x in CMS 4.x after March 2021 #9274

Closed sminnee closed 4 years ago

sminnee commented 5 years ago

If we are to support Silverstripe CMS 4 beyond the end of 2021 (which is likely something we want to be able to do), and we want to remain on a supported version of Symfony, then we would need to upgrade to Symfony 4.4 (see https://symfony.com/roadmap).

On the face of it this seems difficult to do without breaking APIs, but the exposure of Symfony APIs to Silverstripe CMS developers is pretty limited.


Config is the most significant issue: the recommended way of configuring cache is to provide Symfony/Cache implementations. Symfony\Component\Cache\Simple\FilesystemCache, for example, is used. This is exists but is deprecated in Symfony 4.4 ("since Symfony 4.3, use FilesystemAdapter and type-hint for CacheInterface instead"). We could update our built-in configs to use the new API, but existing customisation would throw an E_USER_DEPRECATED error. Perhaps that's acceptable?


We also make use of Symfony in the following places. However, these use cases don't expose Symfony APIs directly for extending Silverstripe CMS and so


All of this is manageable. Another concern, however, is that someone is relying on Symfony 3 in their project and if we then require Symfony 4 there will be a conflict. It's possible that we could have the framework code work with either Symfony 3 or 4, but it would be messy, require duplication of test configurations and implementation. It's questionable whether this is support for an EOLed Symfony 3 would be necessary, or whether such people can simply pin themselves to the SilverStripe 4.x version that still depended on Symfony 3.

chillu commented 5 years ago

+1 for a Symfony 4 upgrade within the 4.x release line, and for not providing hacky backwards compat with Symfony 3. The PHP language and the Composer system unfortunately don't make it easy to run parallel library versions, we're working under these constraints. We need to provide a secure system, and with Symfony being involved in request processing, the only way to achieve that is to stay on supported versions of that library. Worst case, we'll need to make the last SS 4.x minor release compatible with Symfony 3.x a separate LTS release (rather than just the latest SS 4.x minor release), but I'd only be willing to take on this extra maintenance effort if we have concrete use cases where it would otherwise block important use cases (rather than just being an inconvenience).

Looking through https://github.com/symfony/symfony/blob/master/UPGRADE-4.0.md. Looks reasonably benign (famous last words). YAML parsing changes might trip us up. e.g. "Starting an unquoted string with % leads to a ParseException". We document it like this on https://docs.silverstripe.org/en/4/developer_guides/extending/injector/. Could provide an upgrade task.

I've run some checks on our CWP 2.x kitchen sink (pretty much all supported modules):

composer why symfony/class-loader
behat/behat  v3.5.0  requires  symfony/class-loader (~2.1||~3.0) 

composer why symfony/config
behat/behat             v3.5.0     requires  symfony/config (~2.3||~3.0||~4.0)  
behat/mink-extension    2.3.1      requires  symfony/config (^2.7|^3.0|^4.0)    
silverstripe/framework  4.4.x-dev  requires  symfony/config (^3.2)  

composer why symfony/css-selector
behat/mink  v1.7.1  requires  symfony/css-selector (~2.1|~3.0)  

composer why symfony/dependency-injection
behat/behat  v3.5.0  requires  symfony/dependency-injection (~2.1||~3.0||~4.0) 

composer why symfony/event-dispatcher
behat/behat  v3.5.0  requires  symfony/event-dispatcher (~2.1||~3.0||~4.0) 

composer why symfony/filesystem
composer/composer  1.9.0    requires  symfony/filesystem (^2.7 || ^3.0 || ^4.0)  
symfony/config     v3.4.30  requires  symfony/filesystem (~2.8|~3.0|~4.0) 

composer why symfony/finder
composer/composer             1.9.0      requires  symfony/finder (^2.7 || ^3.0 || ^4.0)  
silverstripe/behat-extension  4.x-dev    requires  symfony/finder (^3.2)                  
silverstripe/config           1.0.x-dev  requires  symfony/finder (^2.8 || ^3.2) 

composer why symfony/process
composer/composer            1.9.0      requires  symfony/process (^2.7 || ^3.0 || ^4.0)  
facebook/webdriver           1.7.1      requires  symfony/process (^2.8 || ^3.1 || ^4.0)  
silverstripe/fulltextsearch  3.5.x-dev  requires  symfony/process (^3.2)                  
silverstripe/serve           2.x-dev    requires  symfony/process (^3.0 || ^4.0) 

composer why symfony/service-contracts
symfony/console  v4.3.3  requires  symfony/service-contracts (^1.1) 

composer why symfony/translation
behat/behat             v3.5.0     requires  symfony/translation (~2.3||~3.0||~4.0)  
silverstripe/framework  4.4.x-dev  requires  symfony/translation (^2.8) 

composer why symfony/yaml
behat/behat                             v3.5.0     requires  symfony/yaml (~2.1||~3.0||~4.0)  
phpunit/phpunit                         5.7.27     requires  symfony/yaml (~2.1|~3.0|~4.0)    
silverstripe/config                     1.0.x-dev  requires  symfony/yaml (^2.8 || ^3.2)      
silverstripe/framework                  4.4.x-dev  requires  symfony/yaml (~3.2)              
symbiote/silverstripe-advancedworkflow  5.2.x-dev  requires  symfony/yaml (~3.2)
robbieaverill commented 4 years ago

I'm all for this. It's worth noting that unquoted injector references are deprecated by symfony/yaml at the moment, so support for that might be tricky to retain backwards compatibility for... e.g. FooService: %$MyServiceClass

sminnee commented 4 years ago

@chillu have you been discussing something of this sort with Benn?

sminnee commented 4 years ago

How long should we stick with Symfony 3, then? Should we leave it as late as possible, or upgrade sooner?

If we're looking to leave it as late as possible I would probably say the first 4.x minor release after March 2021 should have this in.

chillu commented 4 years ago

I'd leave it as late as possible, because we've spent a lot of "upgrading good will" in the last two years. In March 2021, we might be in a position where devs upgrading to that new Symfony4 compatible 4.x release to consider a 5.x-alpha/beta instead (rather than the sunk cost - for some - of further upgrades on 4.x)

sminnee commented 4 years ago

Alrighty, then:

Proposal for voting

sminnee commented 4 years ago

@silverstripe/core-team ping - there's an RFC for voting. It's a plan for 18 months in the future, but at least it gives clarity on this point.

maxime-rainville commented 4 years ago

From our experience with the upgrader, there doesn't appear to be an awful lot that has changed between Symfony 3 and Symfony 4. We ended up having a very loose constraint saying you could use either 3 or 4. This was so people installing the upgrader globally with composer would not be blocked if they had other packages that required a specific version of Symfony.

I have no idea if this would be practical on framework or our other core modules.

On the broader question, I think swapping libraries we depend on in a minor release is fair-game.

ScopeyNZ commented 4 years ago

I think swapping libraries we depend on in a minor release is fair-game.

Agreed, as long as we don't directly expose their APIs - For instance, places where we return a Symfony object might be breaking if that object has had breaking changes in their major release.

chillu commented 4 years ago

OK six core committer votes in favour, calling rfc/accepted

sminnee commented 4 years ago

I'd like to pick this up again.

I think that it would be useful to amend support so that ^3 || ^4, or even ^3 || ^4 || ^5, is supported by framework, and consider tolerate the code duplication that this would require.

We would have the option for deleting support for ^3 at some point in the future, but this means that any users of Silverstripe are able to decouple the timing of their Symfony upgrade from that of their Silverstripe upgrade.

By way of analogy, we dropped support for PHP 5.6 some time after we added support for PHP 7.

Furthermore, if we're able to continue supporting Symfony 3, I don't think we need to embargo it until March

+1 for a Symfony 4 upgrade within the 4.x release line, and for not providing hacky backwards compat with Symfony 3. The PHP language and the Composer system unfortunately don't make it easy to run parallel library versions, we're working under these constraints.

I've found that packages that support multiple major versions of Symfony is an increasingly common pattern and the only way out of dependency hell.

maxime-rainville commented 4 years ago

@brynwhyman I think we'll need to bring this in sprint. We rely on symphony for a lot of things and I don't think it's tolerable to be on an unreleased version.

sminnee commented 4 years ago

We're running 3.4, which isn't EOL yet

image
sminnee commented 4 years ago

Although we use symfony/translation 2.8, which is EOL.

brynwhyman commented 4 years ago

@brynwhyman I think we'll need to bring this in sprint.

Yes, we could look to get this in before the likely 4.7 release in December.

sminnee commented 4 years ago

A little more food for thought:

sminnee commented 4 years ago

Note that pinning symfony/config to ^3 is stopping behat tests from running on PHP 7.4

Later 4.x releases of symfony/dependency-injection work with PHP 7.4

sminnee commented 4 years ago

It looks like this might be pretty straightforward: #9698

sminnee commented 4 years ago

Behold, the mind-blowing complexity of the changes required! ;)

diff --git a/src/i18n/Messages/YamlWriter.php b/src/i18n/Messages/YamlWriter.php
index d5917f977..14a6b47ae 100644
--- a/src/i18n/Messages/YamlWriter.php
+++ b/src/i18n/Messages/YamlWriter.php
@@ -29,8 +29,7 @@ class YamlWriter implements Writer
     protected function getDumper()
     {
         if (!$this->dumper) {
-            $this->dumper = new Dumper();
-            $this->dumper->setIndentation(2);
+            $this->dumper = new Dumper(2);
         }
         return $this->dumper;
     }
diff --git a/tests/php/ORM/DataObjectTest.yml b/tests/php/ORM/DataObjectTest.yml
index 242eceba8..b55c6811a 100644
--- a/tests/php/ORM/DataObjectTest.yml
+++ b/tests/php/ORM/DataObjectTest.yml
@@ -143,7 +143,7 @@ SilverStripe\ORM\Tests\DataObjectTest\Company:
   company1:
     Name: Company corp
     Owner: =>SilverStripe\ORM\Tests\DataObjectTest\Player.player1
-  company1:
+  company2:
     Name: 'Team co.'
     Owner: =>SilverStripe\ORM\Tests\DataObjectTest\Player.player2
 SilverStripe\ORM\Tests\DataObjectTest\Bracket:
chillu commented 3 years ago

Releasing this will also resolve a specific quirk in how composer prefers newer but unsupported releases over older but supported ones. In the scenario below, I'd expect symfony/dependency-injection:3.4 to be installed, but it uses an older minor release of the newer major release (symfony/dependency-injection:4.0) instead - a minor release that's no longer supported. Composer of course doesn't know anything about the support status of branches. In 99% of cases, we wouldn't need to communicate this via constraints or conflicts, but in this particular case it's resolved by framework supporting symfony/config:4


composer create-project silverstripe/installer my-project
cd my-project
composer require --dev silverstripe/behat-extension

composer info | grep symfony/dependency-injection
symfony/dependency-injection          v4.0.15 Symfony DependencyInjection Component

composer why symfony/dependency-injection
behat/behat  v3.7.0  requires  symfony/dependency-injection (^2.7.51 || ^3.0 || ^4.0 || ^5.0) 

composer why behat/behat
behat/mink-extension          2.3.1  requires  behat/behat (^3.0.5)  
silverstripe/behat-extension  4.2.1  requires  behat/behat (^3.2) 

composer prohibits symfony/dependency-injection '4.1.0'
symfony/dependency-injection  v4.1.0  conflicts  symfony/config (<4.1)  

composer why symfony/config
behat/behat             v3.7.0  requires  symfony/config (^2.7.51 || ^3.0 || ^4.0 || ^5.0)  
behat/mink-extension    2.3.1   requires  symfony/config (^2.7|^3.0|^4.0)                   
silverstripe/framework  4.6.2   requires  symfony/config (^3.2)