silverstripe / silverstripe-framework

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

RFC: PHP7 support in SilverStripe 3 #6759

Closed sminnee closed 7 years ago

sminnee commented 7 years ago

Problem: we only have ~15 months between SS4 stable release and SS3 end-of-life.

Right now SS3's end-of-life is tied to the end-of-life of the PHP version that it requires - 5.6. This is December 2018. We're targeting an October 2017 release for SS4 stable, which only leaves a 15 months window for people to upgrade.

For comparison, we had 33 months (2 years, 9 months) between 3.0 being released and 2.4 being end-of-life'd.

15 months is too short a window and I would like a minimum of 2 years.

Solution: PHP7 support in SS3

In order to resolve this, we want to support PHP7 in SS3. This could start at 3.6. However, as this requires an API breakage, we would be breaking semantic versioning to do this.

Having discussed it as a core team, we don't want to have semantic versioning be something that we do when convenient, and so we've come up with another solution: we will put the breaking version of SS3 into a separate package, silverstripe/framework3lts

What this will means is that the version list will look a bit unusual:

If you don't want to worry about this, you can skip straight from 3.5 to 4.0. But if you want to get onto PHP7 before upgrading to SS4, you will have a way forward that doesn't require pulling in patches to your system.

Specifics

What are the API breakages?

They're pretty small:

So ordinary use of the field type classes (i.e. by referencing them in $db and $casting) will continue to work, only direct references to these classes will break. The most likely place that this would happen is if someone has a module that provides a new special field type that is a subclass of Int or Float. This is pretty unusual and so shouldn't come up often.

We can provide a .upgrade.yml file to help with these renames.

Outcomes

Most things should be okay:

There is one small caveat:

Then you will run into issues, and you will need to contribute a 3.6+ version of the module as well as patching the original version of the module to the 3.x-3.5 only.

However, we expect that this situation will be rare as direct references ot the Int and Float classes are rare.

Impact on support timeline

As a result of this the support timeline would be this:

timeline

Alternatives

Just go with 15 months overlap — SS3 is old enough

For a site that's maintained by an in-house developer or a small project, 15 months might seem like plenty of time. But in larger companies we've found that major-version upgrades will often need to be scheduled into cap-ex budget in the following year. And if the upgrade is coupled with a major redesign it could be a significant project. In this context, 15 months will quickly pass by.

Just break semver — it's just 1 little class

This is an option but the above approach means that there's an explicit step needed by a site maintainer to switch to the new track and receive 3.6+ updates. It felt like a better compromise than simply breaking semver.

This is one of the more contentious points in the discussions I've had, but ultimately, semver is about trust and so we wanted to avoid breaking it if we could.

Don't support PHP 7 — just let people keep using PHP 5.6

Recommending a "supported version" that only runs on an unsupported version of PHP seems logically inconsistent and doesn't really give people a solution to the problem of continuing to safely run an SS3 site.

Just use composer-patches to apply the change — it works!

For the occasional user wanting to make use of SS3 on PHP7, composer-patches is an elegant hack. But if we're recommending its use more widely it starts to break down. For example:

I wanted to find a more robust solution.

Load different code for PHP7 vs PHP5.6

For example, class Int extends DBInt would be a PHP5.6 only class. I'd like to avoid this for a few reasons:

dhensby commented 7 years ago

Currently my view is:

Just use composer-patches to apply the change — it works!

At the moment I don't see that PHP 7 support in 3 is important enough to justify either breaking APIs nor running a special fork for.

I am happy to extend support for SS 3 if that's desired, but I don't feel one mandates the other.

The composer-patches solution provides the same "opt-in" inclusion as moving to a new project name for a developer and provides PHP 7 support for those that really want it.

sminnee commented 7 years ago

OK — people can vote on either :-)

silbinarywolf commented 7 years ago

Why not just change to DBInt/DBFloat and then create the classes below for PHP 5.x? Theoretically, that means nothing will break for those that stay on 5.x right?

class Int extends DBInt {
};
class Float extends DBFloat {
};
phptek commented 7 years ago

@SilbinaryWolf assuming of course that the DBxx fields are the only breaking changes for PHP7. Besides, the standard way of achieving your example, is to use Injector, no subclassing necessary:

\SilverStripe\Injector\Injector:
  Int:
    class: \SilverStripe\ORM\Fields\DBInt

...or whatever the namespacing actually is currently in SS4

silbinarywolf commented 7 years ago

They are the only breaking changes for PHP7 in core. I run patched 3.x builds in PHP7 for at least 2 sites.

sminnee commented 7 years ago

What @phptek has described is the approach we'd take. Introducing code that is loaded into a PHP5 .6 install but not a PHP7 install seems problematic and I'd rather take the approaches outlined above.

micmania1 commented 7 years ago

Thinking out loud here but would it be possible to update the class loader to use different classes if the version is php7? ie. classes like Boolean etc don't ever get loaded if its php7 and instead we load classes like \SilverStripe\php7\DBFieldBoolean

Then you use config to map Boolean to DBFieldBoolean like in SS4? Or you could have class Boolean extends SilverStripe\php7\DbFieldBoolean and only load Boolean if its less than php7.

ps. I'm not sure what other blockers there are?

sminnee commented 7 years ago

Thinking out loud here but would it be possible to update the class loader to use different classes if the version is php7?

I really want to stay away from this kind of thing. It increases the chance that things break when switching from PHP 5.6 to PHP 7, necessitates more gloop in the class-loader / manifest, and generally adds unnecessary complexity to the app.

See also: ErrorControlChain

sminnee commented 7 years ago

^ since it's come up a few times I've listed it as an alternative with my reasons not to go with it.

silbinarywolf commented 7 years ago

Fresh thought.

Make upgrading as painless as possible, rather than adding hacks n' such.

sminnee commented 7 years ago

Yeah we could probably use the existing silverstripe-upgrader tool for the class renames; good idea.

We don't have a tool that rewrites PHP5.6 code to PHP7 code and that's kind of out of scope for the project.

phptek commented 7 years ago

We don't have a tool that rewrites PHP5.6 code to PHP7 code

cough Challenge cough

andrewandante commented 7 years ago

TL;DR - PHP7 with SS3 will only let the stragglers straggle longer.

Upgrading to SS4 is something we want to encourage. I don't think that's up for debate. In this way we also want to discourage people from hanging around on SS3. It seems like the problem we are trying to solve with this is giving a client that might drag its feet longer to get moving on to SS4 - in practice, I have a feeling this will just delay the inevitable. Real world examples of SS2.4 still exist, after all.

I'd like to see SS4 using PHP7 as incentive for upgrading. It really ties things together nicely that we can EOL SS3 along with PHP5.6. In the meantime if people really need to tide themselves over with the SS3/PHP7 combo, then the patch exists. I don't think it would be unfair to require a module to extend an old version of SilverStripe to work with a new version of PHP.

Forking to framework3lts feels uncomfortable to me. I can't quite imagine the headspace of someone who is keen to upgrade to PHP7, but not keen to upgrade to SS4. I think it's important to realise that just because PHP5.6 will be EOL, it doesn't mean it will disappear from the face of the earth. The SS3 sites won't suddenly stop working on Dec 18th. I don't see "extending support for SS3" and "SS3 has built-in support for PHP7" as being tightly coupled.

There will always be clients that are resistant to upgrades. This isn't going to solve that problem. You'll just have to get them from 3.1 to 3.6 - and at that point you may as well go all the way. Just don't do the redesign at the same time 😛

gregsmirnov commented 7 years ago

Forking to framework3lts is the best approach to keep the code clean and upgrades less painful. Infrastructure and code should gradually upgrade from corporate point of view. We just managed to get rid of PHP 5.2 and there are some SS 2.4 sites still with other projects covering all variety of SS versions (3.1, 3.2, 3.3, 3.4, 3.5). As Sam said, the bottleneck is budget and man power.

dhensby commented 7 years ago

@gregsmirnov I don't understand your overall point. Do you feel PHP 7 support is essential for SS 3? Do you feel the support timeline for 3 must be extended?

We just managed to get rid of PHP 5.2 and there are some SS 2.4 sites still with other projects covering all variety of SS versions (3.1, 3.2, 3.3, 3.4, 3.5). As Sam said, the bottleneck is budget and man power.

I don't understand how this provides justification to either increase SS 3 support times or introduce a PHP 7 compat version. You're saying it's taken you 6 years to upgrade off PHP 5.2 since it's end of life but you want PHP 7 support on SS 3..

At your current upgrade rate you'll only be off 5.6 by 2024 and if you're still running a few 2.4 sites then you'll have a big problem getting those working. If you're still running SS 3 sites you'll have to do dev work no matter what solution we pick to get it working on PHP 7. Your steps will be: edit composer.json, composer update, and possibly fix regressions from the breakages we introduce. If you're still running SS 3 sites at that point and need PHP 7 support, there's a solution available to you.

I don't see that you're providing any evidence as to why PHP 7 support in SS 3 is essential.

  1. I'm happy to support PHP 7 if we can do so in a way that doesn't break APIs
  2. I'm happy to increase SS 3 support timelines
  3. I don't see that PHP 7 support is essential
dhensby commented 7 years ago

This issue is essentially made of two parts:

  1. Should we increase the support timeline for SS 3 to provide more than 15 months of overlap between v3 and v4?
  2. If so, is it essential to provide PHP 7 support in SS 3?
    • If it is essential, why is composer-patches solution not satisfactory?

If people support the idea of either breaking APIs or adding a separate fork to support PHP 7 please can they justify why they feel PHP 7 support is essential.

kinglozzer commented 7 years ago

I can't quite imagine the headspace of someone who is keen to upgrade to PHP7, but not keen to upgrade to SS4.

Have a sneak peek into mine 😄.

We maintain well over 100 SilverStripe 3 websites from a variety of sectors, which I can group like this:

SS4 won’t be stable for a while, so we still have projects in the pipeline for SS3: if we put an SS3 site live in mid-2017, we can’t reasonably expect clients to pay a significant sum around 12 months later to get an upgrade ready in time.

What I’m saying is, come Dec 2018 and beyond we will have many sites running SilverStripe 3 - that’s inevitable. Sure, we can leave them running on PHP 5.6 - just like we could’ve left our SS 2.4 sites running on PHP 5.2 - but to do so would be foolish.

I don't see that PHP 7 support is essential

It’s essential for our business because we want to keep our websites as secure as possible, for as long as possible, within our clients’ budget constraints. For those clients that don’t have the money for an upgrade our options are limited: switch to PHP 7, or keep using PHP 5.6 into 2019 and cross our fingers.

Is it essential for SilverStripe Ltd to provide long-term support for SS3 + PHP 7? I can’t answer that. All I can say is that one way or another we will have SS3 sites running on PHP 7 - if it can be done via the OSS project, with SilverStripe Ltd’s official support then great! If not, we’ll be using the composer patches solution.


Edit:

All options considered, my order of preference is:

  1. SS Ltd officially adopts the composer patches approach
  2. Break semver (I do believe this is a genuine one-off)
  3. SS Ltd extends 3.x LTS, but ignores PHP 7 support*
  4. Everything else

*Nobody has been expecting SS Ltd to extend SS3 support, nor has anyone been expecting SS Ltd to provide SS3+PHP 7 support. Anything offered is a bonus, very gratefully received 😄❤️.

dhensby commented 7 years ago

In terms of defending my point of view, I won't say more than this on the issue:

Providing PHP 7 support doesn't give anyone anymore time to get off of PHP 5.6, the end-of-life date for PHP 5.6 is set and won't change. Agencies who "refuse" to run on end-of-life software will still have to upgrade all their client sites to our PHP7 compat version of SS 3 and regression test all those sites in a short period of time regardless of our support timelines beyond 12/18. I'd wager that no agency will do this in time.

If anyone here has a site running on SS < 3.5.2 or PHP < 5.6.30 then you're running vulnerable software and it's a tad disingenuous to proclaim how essential PHP 7 support is for "security" reasons and how running PHP 5.6 after 12/18 would be unacceptable. There's a disconnect between what people want (PHP 7 support), the justification (we need it for security when 5.6 is EoL), and the reality (not all your sites will be upgraded to the PHP 7 compat version of SS even if it becomes a reality).

If you currently have a client on 3.0 or 2.x who won't sign off on upgrading to 3.5 then what makes you think you'll get sign off to go to 3.6 and PHP 7? Even getting clients to sign off on upgrading from 3.1 to 3.5 is hard, again, what makes getting them to agree to 3.6 any easier?

Lastly, there's a difference between it being essential to have a way of running SS 3 on PHP 7 vs SS providing this support through an LTS branch/fork. Having the option to upgrade sites that can afford it or need it is great and we have a solution for that - composer-patches. Otherwise we're just arguing for PHP 7 support because it'd be cool but ultimately it won't be taken advantage of on most client sites.

UndefinedOffset commented 7 years ago

In some ways I like the idea of renaming Int to DBInt (same for Float) and having an empty stub then in the construct of Int and Float have it throw a deprecation warning. Injector could then be used to replace Int and Float where they're not directly instantiated using new Int() for example which I think would get around the deprecation warning. It's not ideal but it does keep from breaking symver as well as changing the minimum PHP requirement. Which as many above have pointed out there's allot of clients that for some reason budget or otherwise won't upgrade to 3.5 little own spend the time/money to upgrade their hosting to even PHP 5.5.

Even though we all know how important it is to keep PHP up to date, there are also other PHP tools some clients have that would run into issues going between versions especially with the breaking changes in PHP 5.4 (I think it was). In some cases just upgrading the SilverStripe side may not be the end of what needs to be changed and for some customers that could balloon costs significantly and likely out of the realm of possibilities.

Edit: There would need to be a check to see if the class Int or Float exists, because it sounds like PHP 7 defines the Int and Float classes (otherwise this conversation wouldn't be needed).

jonom commented 7 years ago

There is one small caveat: If you have upgraded to 3.6 (...) modules don't have a special 3.6+ version yet (...) Then you will run into issues, and you will need to contribute a 3.6+ version of the module as well as patching the original version of the module to the 3.x-3.5 only.

I don't think you can exactly patch the original version to be 3.x-3.5 only, and don't think that second step should be required anyway. AFAIK you can't retract or modify tags/releases after they've been published, so if you have for example a 1.0 release tag that has a loose requirement like silverstripe/framework: ^3.1, and then you publish a 1.1 tag with a narrower requirement like silverstripe/framework: >=3.1 <3.6, Composer will still see the 1.0 release as being viable for compatibility with SS 3.6. However, if you're publishing SS3.6 under a different package name as suggested then it should be a non-issue anyway as it's essentially a completely different module.

I think the suggested solution would be okay, as long as module authors are responsive about making 3.6+ releases. Realistically though I expect it will slow down SS3.6 adoption quite a bit as some modules won't do this right away (or accept pull requests right away), and then we'll have a conflict with some modules requiring silverstripe/framework3lts and some requiring silverstripe/framework. It will be pretty messy I think.

Leap frog approach

Instead of a separate package name, could we just have a separate release branch on silverstripe/framework for PHP7 support in SS3? This could be identical to SS3 stable but with the Int/Float changes and PHP7 composer dependancy included. Then we could just leapfrog minor releases.

Equivalent releases :frog:
PHP5 SS 3.6.0 3.6.1 3.6.2 SS 3.8.0 3.8.1
PHP7 SS 3.7.0 3.7.1 3.7.2 SS 3.9.0 3.9.1

Yes, technically the API changes break SemVer but it's paradoxical. The minimum composer requirement of PHP7 on those releases means anyone running those releases must have those changes for things to work, so I think in this case it would be acceptable. SemVer isn't perfect and following it to a tee here might not make sense.

lerni commented 7 years ago

For me the composer-patch-solution is easier to understand than the others and it also doesn't violate SemVer. In practice I just came across one issue with a module and php7: You cannot have same class- & function-names, but I think module maintainers would accept PRs, since this needs to be fixed anyway in the long run.

There a two workarounds for composer-php requirements. You can run composer with "--ignore-platform-reqs" or have it in composer.json - see also: https://getcomposer.org/doc/06-config.md#platform

I would prefer if the composer-patch would be officially supported and SilverStripe version-history stays clean & tight.

sminnee commented 7 years ago

I guess one key counterpoint to all of this is that Debian Jessie LTS extends until April 2020 (https://wiki.debian.org/LTS)

So that would give another alternative to solving the underlying issue (the short overlap of SS3 and SS4 stable support)

Support SilverStripe 3 on PHP 5.6 until October 2019, assuming that users will rely on Debian Jessie LTS or another LTS distributions for PHP support

This is another approach, and could be done alongside the composer-patches solution if desired, so people can choose.

The only issues is that Jessie LTS is a community-driven project rather than part of Debian core and we don't yet have confirmation that they're going to provide LTS patching for PHP 5.6.

NicoHaase commented 7 years ago

Just to give another point of view: we work for many companies in Europe and maintain our own CMS. It has some really old code, but we were able to achieve PHP7 compatibility in reasonable time - result: the websites got much faster.

Some weeks ago, I tried to upgrade my own private server to PHP7. My self-written CMS for a really special purpose with code that is mostly years old only neede minor steps to work. But you want to tell me that a current CMS like SilverStripe won't even run on a freshly installed Ubuntu server? PHP7 was released more than a year ago, that there will be some more reserved keywords was not a secret since Feb 2015, according to https://wiki.php.net/rfc/reserve_more_types_in_php_7, and you want to hang on to not supporting it for even much more months? If Silverstripe finally achieves compatibility in late 2017, developers still need to fix their projects until they are ready to run on customers servers which will take even more time. So I really encourage you to find a solution in between before customers leave.

To be honest: I'm happy not to have a paid SS project at the moment where I need to explain my customer that PHP7 runs rocket-fast, everyone encourages to do this upgrade, but oh no, your CMS does not yet support it and will not for months and months.......

sminnee commented 7 years ago

On reserved works: SS3 won't ever support PHP 7.2, because getting rid of the Object class is substantially more invasive. PHP 7.1 and 7.0 only have conflicts with Int and Float.

wernerkrauss commented 7 years ago

@NicoHaase there is a really simple composer patch module for getting SS3.2+ running with php7.0, see packagist .

As @lerni pointet out you have to use composer with --ignore-platform-reqs and be sure to use same php version on CLI and webserver, otherwise silverstripe-cache will fail for some reasons and you see a beautiful white screen of death.

wernerkrauss commented 7 years ago

regarding semver: are there any known direct subclasses of Int and Float in modules that might break when we silently rename them and re-add the functionality with Injector like in the known composer patch?

andrewandante commented 7 years ago

I can get behind @lerni - if we say our recommended approach for PHP7 is to upgrade to 4, followed by using the composer patches - that's as good as building it straight in. If we need to, we can bring the patches module up to our supported standard (assuming it's not already, which it might well be). That seems significantly less effort, and a cleaner method progressing from here.

As has been mentioned, the issue gets muddied with modules that are for SS3, but would break with PHP7 - but that problem will need to be solved anyway. The composer patches should be seen as just that - patches.

lekoala commented 7 years ago

I'd really like to see SS3 support PHP7. Upgrading to SS4 is a significant change (for some websites I manage, it's going to require lots of change). Even if renaming two classes breaks semver, it's totally worth it if properly documented. Nobobdy is going to have any trouble fixing these issues.

dhensby commented 7 years ago

@kinglozzer and I have been talking about this issue a little bit outside of this thread. We've come to the agreement that we'd support a polyfill in core code. This involves:

  1. Renaming Int and Float to DBInt and DBFloat respectively
  2. Creating classes that extend these newly named classes to maintain BC (eg: class Int extends DBInt). These classes would be wrapped in if (PHP_MAJOR_VERSION < 7) conditions.

You can see the code here: https://github.com/silverstripe/silverstripe-framework/compare/3...kinglozzer:php7-ss3

The reason we support this is:

  1. It provides PHP 7 support out of the box
  2. It doesn't break any code for existing sites
  3. the only code that won't work on PHP 7 will be code that needs changing no matter what our approach is

In other words, code bases that could run on PHP 7 but for our code code will now work, code that would need to be edited to run on PHP 7 will still need to be upgraded to run on PHP 7.

By adding PHP 7 support we are saying our code works on PHP 7 (and it will), if there are issues on PHP 7 it's most likely because of their code or thirdparty modules so I'm comfortable saying that we support PHP 7 even though people could use the code base to write PHP 7 incomparable code.

We have to be prepared that we'll never support PHP 7.2 (or some other version of the 7.x line) as this RFC raises the possibility of reserving the object keyword (https://wiki.php.net/rfc/object-typehint?s[]=object). As and when we know if that get's approval for a PHP version we'll have to update the constraints of our software.

jonom commented 7 years ago

Sounds pretty smart to me

sminnee commented 7 years ago

@dhensby @kinglozzer I'm okay with the approach in principle; can someone do this as a PR with an update to the .travis file so we can see the effect on our test suite?

dhensby commented 7 years ago

over to @kinglozzer for that :)

sminnee commented 7 years ago

@silverstripe/core-team can you all please vote on Dan's comment outlining the solution that Loz and Dan came up with? If we're good to go, then we can focus on reviewing & merging #6771.

flamerohr commented 7 years ago

Closing this since pull requests have merged and we might enter a 3.6-beta to before releasing a 3.6