SPIKE Figure out best way to remove deprecated API from codebase and provide a transition path

[maxime-rainville]

CMS 4 has many API marked for deprecations.

Objectives

• We agree on a strategy to remove deprecated APIs.
• The approach provide a off ramp for people upgrading from CMS4 to CMS5.
• The approach is documented.
• There's clear step in follow up cards for how to remove the deprecated APIs.
• The approach can be carried forward for the CMS5 to CMS6 transition.

Timebox

1 day

Note

• The Symfony approach to deprecation seems pretty sensible and could probably work for us.
  • @jakxnz was the one who suggested tho Symfony approach here. Might be worth having a chat with him.
  • They also have a toolbar which displays deprecations - we could add something like this to the lekoala debugbar.
• Doing tho actual removing is not part of this card.
• Identifying other APIs we might want to deprecate is not part of this card.
  • Though note that the graphql legacy schema in various modules should be included.
• Any deprecation warnings we have added via Deprecation::notice() should have a version set to 5.0.0 (i.e. the version the API will be removed in)
  • silverstripe/framework has Deprecation::notification_version('4.0.0'); in its _config.php file. Setting this to '5.0.0' will mean any deprecation notices sent via this class will be thrown as E_USER_DEPRECATED warnings on the front-end.
  • Developers can disable this via Deprecation::set_enabled(true); in their project's _config.php file or using SS_DEPRECATION_ENABLED="0" in their .env file.
  • This does not cover the @deprecated phpdoc - we will need a separate mechanism to throw warnings for these.

[emteknetnz]

I've spent some time investigating this, my recommendations:

Don't use the symfony approach Don't use symfony trigger_deprecation()

I've investigated the symfony approach to deprecation via the use of the trigger_deprecation() function in symfony/deprecation-contracts. While it's provides a nicer output than the Silverstripe Deprecation::notice(), particularly when combined with symfony/phpunit-bridge, It's not going to easily work in our codebase.

The problem with it is that you can only do class level deprecations, while it fails to do method level deprecations. The reason for this is the output given when running symfony/phpunit-bridge is THE ERROR HANDLER HAS CHANGED! - this happens because something in the code, somewhere, is running set_error_handler($someHandler) without reverting it via restore_error_handler(). I tried putting a break point in literally every custom error handler method I could find, including all vendor files, however I still couldn't find out what was hijacking this.

So failing that, while Deprecation::notice() provides a fairly ugly console output, it's still functional enough to get the job done

Work to do on our code base

The @deprecated docblock currently does absolutely nothing. I recommend we create an automated code scanner/writer to find all instances of the @deprecated docblock in our code and convert them to `Deprecation::notice(). We have plenty of experience with this having taken a similar approach with the PHP 8.1 code-writing along with the POC to strongly type everything.

Use the following syntax for deprecating classes, methods and parameter types:

<?php
use SilverStripe\Dev\Deprecation;

class MyClass
{
    public function __construct()
    {
        // deprecate an entire class
        Deprecation::notice('4.11', 'Will be removed in 5.0, use MyOtherClass instead', Deprecation::SCOPE_CLASS);
    }

    public function myMethod()
    {
        // deprecrate a single method
        Deprecation::notice('4.11', 'Will be removed in 5.0, use myOtherMethod() instead');
    }
    
    public function myMethod($param)
    {
        // deprecate a parameter type
        if (!is_string($param)) {
            Deprecation::notice('4.11', '$param must be a string in 5.0');
        }
    }
}

Workflow to deprecate things in our codebase

1. Create PR to add the deprecation warning in 4 branch
2. Once merged, merge-up to 5
3. Create PR to delete method in 5
4. Create PR to silverstripe/developer-docs 5.0.0 changelog adding an entry to the "Deprecated" section that mentions the code required and what code should be used instead

Steps to add to the CMS 4 -> CMS 5 upgrade guide

• In the projects _config.php, add the following: SilverStripe\Dev\Deprecation::notification_version('5.0.0');

[GuySartorelli]

I think what was meant by "the symfony approach" is that any time we replace API in CMS 5, we deprecate the old API and provide its replacement in CMS 4. This means that someone on the last minor of CMS 4 can resolve all of their deprecation warnings and then when they upgrade to CMS 5 they'll have little to no changes required.
It's obviously not feasible in all situations though, such as when the replacement lives in a third-party dependency.

[GuySartorelli]

On the whole I agree with the recommended approach - though I think we should set SilverStripe\Dev\Deprecation::notification_version('5.0.0'); in framework (it's already in framework, just set to an earlier version) in the final 4.x minor release. Projects can disable deprecation notices sent through the Deprecation class if they want to in their projects.

[emteknetnz]

Yes that's correct about the 'not always feasible' bit. Just to clarify, part of my recommendation is to guide projects that are upgrading to put SilverStripe\Dev\Deprecation::notification_version('5.0.0'); in _config.php in the CMS 4 project. This will give them a list of things that need to will need to be changed. There won't always be an immediate replacement as they haven't yet upgraded to CMS 5, though at least it tell them what they'll need to do.

[maxime-rainville]

That looks broadly sensible.

@deprecated

On the question of the @deprecated PHPDoc tag, while they don't affect the execution of the code in any way, most IDEs will call out when you are calling a deprecated method.

So I think it's worth preserving the existing @deprecated tag in 4 and would be incline to add any missing ones.

Automation

I'm a bit ambivalent about how much we can automate this. I suspect there will still be a lot of manual work. For example, we are still calling a lot of those deprecated method within none deprecated code. If our end goal is that people can install the latest CMS4 minor and use that to squash deprecation warning, we need to make sure that our own code doesn't call deprecated method.

Also, what do we do with Unit test for deprecated method in CMS4? Presumably we have to keep them around, but will they pollute the output of our CI?

Might be worthwhile picking 10-15 deprecations to see how easy or not easy it is to automate.

Any other alternative approach

Did we consider any alternative approach or tools? I know our first foray into Rector was less than conclusive, but it might be worthwhile having another go at it in this card or in follow up card.

[maxime-rainville]

@silverstripe/core-team Any views on this?

[GuySartorelli]

what do we do with Unit test for deprecated method in CMS4? Presumably we have to keep them around, but will they pollute the output of our CI?

We could disable deprecation warnings globally during tests by adding something into SapphireTest

[emteknetnz]

So I think it's worth preserving the existing @deprecated tag in 4 and would be incline to add any missing ones

Yes that makes sense

I'm a bit ambivalent about how much we can automate this.

The automation portion is mostly just to add Deprecation::notice(<message>); where there is a @deprecation in a method or class. I don't see that are requiring much manual work.

The manual work of actually updating our code to use the new API's is totally separate.

Also, what do we do with Unit test for deprecated method in CMS4?

We have the "current version" set to 4.0.0 in framework, meaning that Deprecation::notice() does not fire. As part of the upgrade guide we say "set this to 5.0.0" on your CMS 4 project, and it will surface deprecation warnings.

we need to make sure that our own code doesn't call deprecated method.

It's not going to be possible in many cases, because the new API won't be available in the CMS 4. The best we can really do is say "change your code to use this new API that will be available in CMS 5"

Did we consider any alternative approach or tools? ... Rector ... it might be worthwhile having another go

No, my experience with rector was truly awful. It's also fundamentally the wrong tool for what we're doing here.

My recommendation for automation is consistently use Deprecation::notice(), that's it. In terms of code scanning/writing that's pretty basic and quickly rolling our own one-off tool of the back of other tools that we've created will be by far the quickest approach, rather than having to learn another tool that may not do what we even want it to.

[maxime-rainville]

we need to make sure that our own code doesn't call deprecated method.

It's not going to be possible in many cases, because the new API won't be available in the CMS 4. The best we can really do is say "change your code to use this new API that will be available in CMS 5"

That makes sense. To the extent that we're saying "this deprecated method is no longer suitable ... use this other one instead or don't use it at all", I think we should be making an effort to make sure our code doesn't rely on it. But I appreciate their will be scenarios where this is impractical.

For the scenario where a test is specifically aimed at a deprecated method, could we do something like "skip this test if current_version is set to 5.0.0". Then we could turn on deprecation warnings for unit test to help us figure which part of the non-deprecated code is calling deprecated method.

For CMS6, I think that we should set a clear expectation that if we mark something as deprecated, it really shouldn't be used in our own current code base anymore.

[maxime-rainville]

I think those 3 cards cover all the outcomes from the SPIKE.

• Document how deprecation works developer-docs#82
• Standarise deprecations in CMS4 for framework #10500
• Remove deprecated code in CMS5 for core #10501

[maxime-rainville]

All done. We have clear next steps.