Implement utility method for invoking backward compatible code

I was on the fence, mostly because I'm unsure of the performance impact of encouraging closures which may live on the hot path. But I realize that the overhead is probably negligible given all the function calls already happening for a typical request.

I'd be all in on this if:

1. We make proper use of PHPStan from the get-go to help people with types. My proposal is

/**
 * @template Current
 * @template Deprecated
 *
 * @param string $version
 * @param callable(): Current $current
 * @param callable(): Deprecated $deprecated
 *
 * @return Current|Deprecated
 */
function backwardsCompatibleCall(string $version, callable $current, callable $deprecated) {
  return version_compare(DRUPAL_VERSION, $version, '>=') ? $current() : $deprecated();
}

2. We build a PHPStan-Drupal rule that can check our declared Drupal support and easily tell us when to replace the backwardsCompatible call with the current call :D (I think that's a much easier rule to maintain than checking every possible if-statement with a `version_compare` in it.

Well, as discussed earlier, this is extremely helpfull to get into core for the cycle to Drupal 11. One thing we might need to investigate is performance though. Not sure how many BC paths there are in core, but although callable is kinda clean, the other options to call external functions is faster, but perhaps not enough.

Also, shouldn't we handle arguments here also? Wouldnt we need to to call those callables with arguments sometimes? Not sure how aanlysis would react on that would we could have something like $args and unpack (...$args) that in the backwardsCompatibleCall.

Hmm, the arguments could live in the anonimous function couldn't they...

For core there are no core-version-specific bc paths, so it can't use this pattern, but that also means there's no inherent performance implication to adding it, it'd just be about whether a contrib module is using it in the critical path.

Valid point.

While looking at how this would work, and trying to think up tests, I did find a possible weirdness.

The version_compare function in PHP compares versions, but does resolve dev versions below released versions. Also, it cannot parse 10.2.x-dev for example.

This is mostly a problem for tests I'd recon, and if someone runs a dev version of core, things would break then? I don't really see another way to get the released version of core in the dev branch. Also, the current version is 11.x-dev.

1. Should we even handle this?
2. Should we only support release branches?
3. If on a dev branch should we always just return $current()?
4. If a version is not parsable, should we throw an exception?

Can we normalize 10.2.x-dev to 10.2 maybe?

We could, but rather not do that by default since that would be overhead. Also a check on the last few characters which is pretty fast though.

But also, what would the \Drupal::VERSION be when we work with a main branch? Will this be the same? Will this be main? Will this be next major? Will this be next minor?

@bbrala I think we need to do #3364646: Add a branch alias for 11.x for 11.x, once that is in, the 'version' will be 10.2.x-dev, and then if that works, it'll also work for 'main' - so we'd need to increment Drupal::VERSION every six months with whatever the next branch to branch off 'main' is going to be.

That sounds good. I'll have a stab a that so we have something to shoot at.

@mglaman I'd probably not so the whole parsing dance, but check if ends with .x-dev (very cheap) and if so remove it. This is relatively cheap, and then we still use version_compare, which feels nicer to keep. I think @catch also means that main will not be a drupal version, but it will be major.minor.x-dev when we start using main. Until then, this same logic would result in a version comparing to 11 which should parse fine also.

In other news
I've been thinking about this a little more, and was thinking about testing. If we want to test this (even though this is a pretty simple method) we'd need a way to mock the version. This is a bit hard when letting the code live in \Drupal. Secondly, when i look at the goal of that class: "Static Service Container wrapper." Which makes this method feel very out of place.

I was then looking at where this might live if not in that class. Wouldn't an Utility class make more sense? I understand utility classes are a bit of a codesmell in some instances, but the fact it lives in \Drupal really feels wrong.

Some possible alternatives:

1. \Drupal\Core\Helper\Deprecation::backwardsCompatibleCall
2. \Drupal\Core\Deprecation::backwardsCompatibleCall
3. \Drupal\Core\DeprecationHelper::backwardsCompatibleCall

This would also mean we can mock the constant for testing.

I've made an implementation in a way that feels right to me.

1. I've decided to use the Drupal\Core\Utility namespace for the helper.
2. I've added a static function normalizedDrupalVersion to the helper so testing is a little easier.
3. Added unit tests for a relatively wide range of testcases. Hopefully that covers all the bases

Setting this issue to NR so we can move along with something concrete to talk about.

One of the questions i might have; should .x be rewritten to .0 or to .99? Or is that sematics. Deprecations to not appear in patch releases right? So something is always introduced in x.x.0, therefor the whole minor version should be .0 always when on a dev version?

Since #3364646: Add a branch alias for 11.x has been merged, the 'main' discussion is deprecated i think :)

I think the implementation looks good! On Slack we discussed delegating the version normalization to Composer. However the needs of Composer are slightly different to our own in how we want to treat `-dev`. We also expect this code to be usable in a hotpath, which Composer's more resilient semantic versioning isn't. I'm in favour of using Björn's code that is simple, performant and covers our use case :D

The code in the MR looks good and I think the test coverage is sufficient. I do wonder why we chose extending the class we're testing. Is it not better to mock just the method we want to change? That way we can also set an expectation on the protected method being called, that allows us to change the internals of the class and provides a more helpful test breakage if we no longer call the method the test relies on. (In the current scenario we could stop using the protected method and it might be unclear why our tests fail).

I was thinking about whether we could annotate any PHPStan types to make it clearer that the version string should be a version. PHPStan allows us to limit it to specifically \Drupal::VERSION but since it's a changeable argument that's probably undesirable (and is not satisfiable in the test). PHPStan currently doesn't have string-templates for types (that's a feature we should steal from TypeScript) but it does have non-empty-string here.

I don't know what coding standards currently allow, whether we could do @param non-empty-string $version or whether we can add a separate @phpstan-param non-empty-string $version at the bottom to help PHPStan users :D

... I do wonder why we chose extending the class we're testing. Is it not better to mock just the method we want to change? That way we can also set an expectation on the protected method being called, that allows us to change the internals of the class and provides a more helpful test breakage if we no longer call the method the test relies on. (In the current scenario we could stop using the protected method and it might be unclear why our tests fail).

This is true, I ended up like this because i couldn't make it private and mock it, that would requite visibility changes, thus it needed to be protected. Then I thought, this way I minimize mocking needed. But your argument that we can mock that method then set expectations is a good one, since that would indeed ensure we are not secretly removing the dependency.

I was thinking about whether we could annotate any PHPStan types to make it clearer that the version string should be a version. PHPStan allows us to limit it to specifically \Drupal::VERSION but since it's a changeable argument that's probably undesirable (and is not satisfiable in the test). PHPStan currently doesn't have string-templates for types (that's a feature we should steal from TypeScript) but it does have non-empty-string here.

Hmm, if we mock it, we don't need the argument. But then the normalization is pretty untestable since we cannot influence the version used. We cannot mock a class constant and we cannot get a Drupal version from a method (although there is discussion about something similar in the linked issue you provided). The fact the API is smaller if it does not allow for an argument is something i would like though.

Removed normalizedDrupalVersion method in favor of a protected var with Drupal::VERSION as default value. Since mocking static methods is not possible this is a way to test without introducing an extra api surface with the normalizedDrupalVersion static method.

So, the api surface is minimized a little more, inlines the normalization in the method which removed the need for the extra class. Unfortunately mocking cannot mock static methods all to well, so this seems like the most clean way to test. The testing double now doesn't overwrite any code that is used to perform the action, only adds a setter to the class.

@catch i do understand there is some overlap in the related issues, but i would prefer to not wait until that is in. We could always just change this when those land.

On order to make sure we notice i added a new test that checks current core version against a change introduced in 9.5.0 and 99.99.99. If \Drupal::VERSION wil be removed, this will trigger a failure since that constant would then be removed.

Wrote a CR. :)

Thanks so much for this! 💗🙏 I love to try to support as many versions of core as possible, and this will make it much easier. Tagging for DX.

The CR mostly looked great, thanks! I made a few slight edits to the title and content.

I spotted a few trivial "fixes". Easier to just push commits to fix them than open new MR threads. 😅 Leaving RTBC since they're so trivial, but if anyone wants to re-review, please do.

Thanks again!
-Derek

Changes are indeed minimal, thanks for those :) No need to go back to NR

Very nice, but I think at least one of my added comments NW.

Added credits for mglaman for posting and reviewing and slacktalks

Kingdutch for review.

Dww for small optimizations and review

Mondrake for review

Catch for related issues and partipation

1. declare(strict_types=1); can be added to the test file, too.

2. What is the purpose of the $version argument, besides testing? I suppose normally runtime code calls would have \Drupal::VERSION there. So wouldn’t it make sense to have a wrapper method that does not have $version as an argument, retrieves \Drupal::VERSION, and passes it to backwardsCompatibleCall()?

3. I was thinking if we also add a $removedVersion argument, we could check if the current version is greater or equal than that, and throw a deprecation in that case. So when a new branch is opened, we immediately find where code should be adjusted to make the call to new method straight. It would require adding an ignored deprecation in the relevant file, but then in followup issues the job would be done.

Is there at least one example in core that can be converted as part of this issue?

1. True. Simple minimal fix.
2. Also; isolates the class from core more, also enables projects to check for their own version if they want. But most impantly is the fact the class is less prone to extention and the api surface is 1 single method.
3. I think this should stay as is, it's a helper to allow projects to support current major plus next, core only really has 2 supported versions always.

In regards to your next comment, no this won't be used in core probably since there never really a compatibility layer as @catch also mentioned earlier in the issue.

First, maybe $normalizedVersion = str_ends_with($version, '-dev') ? str_replace(['.x-dev', '-dev'], '.0', $version) : $version; should be preg_replace('/(\.x)?-dev$/', '', $version) instead?

Now I typed the following up to suggest to be included in the doxygen, recognizing the advantage of the function call being an expression vs if which is not. But we are on PHP 8 which has match so my doxygen suggestion would be

Instead of 
@code
$roles = match(version_compare(\Drupal::VERSION, '10.2', '>=')) {
  TRUE: Role::loadMultiple($account);
  // @phpstan-ignore-next-line
  FALSE: user_roles($account);
};
@endcode

One could use the more expressive:

@code
$roles = \Drupal::backwardsCompatibleCall(
  version: '10.2',
  current: fn () => Role::loadMultiple($account),
  deprecated: fn () => user_roles($account),
);
@endcode

But this lead me to think, is this really any simpler? Couldn't we simplify and make the match more expressive instead?

$roles = match(\Drupal::needsDeprecatedCallBefore('10.2')) {
  \Drupal::CURRENT: Role::loadMultiple($account);
  \Drupal::DEPRECATED: user_roles($account);
};

Where \Drupal::CURRENT and \Drupal::DEPRECATED are just TRUE/FALSE and needsDeprecatedCallBefore is an appropriate wrapper of version_compare with the VERSION constant. And we don't need // @phpstan-ignore-next-line because -- I hope this is as easy as I present it here -- \Drupal::DEPRECATED tells a deprecated call follows. We could put this pattern in the doxygen of needsDeprecatedCallBefore and every change notice where something gets deprecated. Hopefully it is also possible to find the parent expression and automatically replace the whole match expression with the value of \Drupal::CURRENT when one wants to clean this up in the next major (edit: from a brief check of nikic/PHP-parser, it seems to me this would require looking at the arms of a match and if they are \Drupal::CURRENT and \Drupal::DEPRECATED then replace the entire match -- but it seems to be possible indeed).

What do you think?

Although you probably could using maych, the pattern I proposed will be easier, also when working with rector I think. This change is to facilitate backwards compatibility calls for projects, core won't use them. This also enables easier bc patches for project update bot through rector.

I'd rather change as little as possible in core and couple as loosely to core as possible, since this is not to be used in core, just supplied to help the ecosystem. Therefor things like new constants are something I'd rather avoid.

Regarding the preg_replace, I used the sting functions because of the performance optimization, and my rule of thumb, if I don't need a regex I won't use one. I also mentioned this in the mr comments I think.

So thanks for the suggestions, but I feel the current setup is better for the goals we have with this code.

A working (in unit test) proof of concept of this in Drupal Rector.

Updated IS

If this is meant to be independent from core, then I think the class should live under Component\Utility, not under Core\Utility.

You are completely right @mondrake. Quoting from the readme of the Component namespace.

Drupal Components are independent libraries that do not depend on the rest of
Drupal in order to function.

Components MAY depend on other Drupal Components or external libraries/packages,
but MUST NOT depend on any other Drupal code.

Which we realized by adding the extra version parameter.

1. Moved the composent to the Compoenent namespace.
2. Updated the CR
3. Updated the IS
4. Ran tests and codestyle checks locally

Think everything has been adressed :)

The request by mondrake to move this to the right namespace has been resolved. My questions about the comments have also been resolved which makes it clearer for people that this is not Drupal core specific. The changes to the class also improved testability and the test coverage is good. With all that together I think this is ready to be merged!

I've slightly changed the related change record to make it clearer what the change from version_compare looks like. The previous version of the change record used different version numbers as arguments which might be confusing when trying to use the CR to update your own code.

I think the test should use fast-call syntax https://php.watch/versions/8.1/first-class-callable-syntax to negate effect of condition
Or at least fn() => as #36 suggested

Additionally 8.2 changes https://wiki.php.net/rfc/deprecate_partially_supported_callables IMO is the reason to have example inline with current best-practices

Also named arguments are helpful here as \Drupal::VERSION must be passed which I find as bad DX (maybe better to change the value via reflection)

Here's interdiff and patch of proposed changes, I did not add to MR to get opinions

Fixed patch, interdiff against MR

+++ b/core/tests/Drupal/Tests/Component/Utility/DeprecationHelperTest.php
@@ -24,14 +31,10 @@ class DeprecationHelperTest extends TestCase {
+        version: $core,
+        introducedVersion: $version,

argument names confusing a bit

I think the test should use fast-call syntax https://php.watch/versions/8.1/first-class-callable-syntax to negate effect of condition
Or at least fn() => as #36 suggested

Ok, i agree on fn() => but the fast callable might complicate the scope checking for PHPStan. I'll change to the arrow functions.

Also named arguments are helpful here as \Drupal::VERSION must be passed which I find as bad DX (maybe better to change the value via reflection)

Hmm, I understand where you are coming from, but this opens up usage for other projects. Also I do like the fact it relies on nothing in Core this way. So I understand it's a little weird perhaps, but it has it's pros. Rather not change that.

argument names confusing a bit

This was the best I could come up with. Al the options i could think off didnt make more sense. Any suggestions?

Some options for the vars:

backwardsCompatibleCall($currentVersion, ...)
backwardsCompatibleCall($projectVersion, ...)
backwardsCompatibleCall($checkVersion, ...)
backwardsCompatibleCall(..., $currentImplementation)
backwardsCompatibleCall(..., $implementedIn)
backwardsCompatibleCall(..., $availableSince)

Naming is hard.

backwardsCompatibleCall(string $currentVersion, callable $currentCallable, string $deprecatedVersion, callable $deprecatedCallable)

Agreed that naming is hard. What about this? Slightly changed order for parameters.

Reading through the MR now, a lot happened here since my last comment, but #49 seems quite good to me?

Interdiff against MR, implements #49

Also there's measurments https://gist.github.com/donquixote/85efcca90056111e967dd14cb1f9de9c from #2982949-113: Introduce CallableResolver to help standardise the DX and error handling for callbacks across various subsystems

Still not clear how to pass arguments for both callables

+++ b/core/lib/Drupal/Component/Utility/DeprecationHelper.php
@@ -0,0 +1,43 @@
+   *    Callback for the current version of Drupal.
...
+   *   Callback for older versions of Drupal.

As component mention of Drupal should be removed

Looks #36 is the most optimal way to pass arguments

#49:

I really like that. It connects the arguments to eachother. It might still feel weird in some cases, but it seems to tell the story as good as it gets. So i updated the code to use that style.

Also removed Drupal from the param comments. And fixed the comment by @catch

@andypost I don't feel the callable syntax you propose in your patch is something i want to suggest to developers in this case. These are a few unknowns in that regards. This code should mostly be used for deprecated calls to core functions and such, and mostly will come from automated fixes by rector i recon. This means we will probably just keep them as tightly scoped in the static call as possible partly because in rector this is way easier, partly because this makes resolving the scope of the code path way easier. But people are free to use any syntax they like ;)

I hope we can now agree on this implementation and pass this to the committers :)

That's a lot of activity since I RTBC'd, I think the previous version was better in most regards though.

1. I agree with Björn that the indirection in the test is not a good idea. fn () => "current" achieves exactly the same result. It looks like we add a lot of noise just to make use of a new PHP feature?

I can see that we're preparing for some deprecations and using a new PHP 8.1 feature, but the test wasn't actually suffering from any of the problems that those things address, so we're just adding complexity.

I see the performance comment in #51 but we're changing a simple arrow function created in a loop for an arrow function created in a loop with another indirection through a function call.

2. I think moving the version parameters around is a step back in readability for non-simple uses. The code that this is to replace is very simple in structure: if {version1} {version2} {then} {else} which maps nicely to the original function definition but no longer works now. An example of a non-simple use case:

backwardsCompatibleCall(
  "version1",
  function () {
     // Some 
     // long
     // implementation
     // for
     // some
     // logic
     // that no longer exists in a library.
  },
  "version 2",
  function () {
    // The old function
  }

This now requires scrolling to compare the versions for the code rather than being able to see it at the start, that also makes automatic grepping/searching of two version strings together more difficult (e.g. I can't do ["'](\d+\.?)+["'],\w*["'](\d+\.?)+["'] anymore).

3. I don't think this is the right place to introduce named arguments since all arguments are required. Especially with modern IDEs I feel like this solves for a problem that doesn't exist. Named arguments are specifically useful if we want to skip defaults but there's no need for that here. All arguments are required arguments.

4. We renamed the arguments to be focused specifically on deprecation, which I can understand from the IS. However the original naming "introduced in" in my opinion makes it clearer that the hidden check is >= and also makes the function useful for library improvements such as

backwardsCompatibleCall(
  php_version(),
  "8.0.0",
  fn () => array_is_list($arr),
  function () {
    if ($arr === []) {
      return true;
    }
    return array_keys($arr) === range(0, count($arr) - 1);
  },
);

(Yes you should probably use function_exists here, but I can imagine there are similar examples where you might not want to do that because you're loading some service in a library that didn't exist before)

edit
I'm actually coming to the conclusion though that we can have both world in a way with your suggestion. I like the 'mental' coupling of var and callable, but that could also be achieved with the naming and keep tell the following story:

"I want to do a ::backwardsCompatibleCall on $currentVersion against $deprecatedVersion resulting in $deprecatedCallable and moving towards $currentCallable"

And there is personal flexibility in argument order with named arguments.

I'm a bit at a loss here to to get consensus though.
/edit

I'm gonna keep the MR updated on my current view of the best idea.

Re 2/3. My compromise here would be to go back to the older version of (, , ,
). Especially if we want to encourage named paramters, that's precisely the point where order no longer matters. So this makes it similar to the old if (version_compare(<version>, <version>) { <new> } else { <old> } if no named parameters are used, and the test can stay as is with named parameters in a different preferred order.

4. I agree it might not get usage outside of Drupal, but I would hope (especially with our move away from hardcoding \Drupal::version) that it can get used for broader usage than just deprecation. I don't even feel that's necessarily looking at a broader view, just another use-case. For example maybe an optional new parameter that's being added in Symfony 5.2

backwardsCompatibleCall(
  symfony_version(),
  "5.2.0",
  fn () => foo("bar", "non-default-baz"),
  fn () => foo("bar"),
);

Comment got eaten by d.o... recommenting

Re #57.4 - exactly because arguments can be different for old and new versions

about naming - we using "deprecated in" all over codebase (SF needs to check)
and there was an idea to create deprecated.module in core few times

The reason for 8.1 is because it affects performance and 8.0 security support expire in 3 month https://www.php.net/supported-versions.php

To run new code on old PHP versions there's symfony/polyfill-php8* packages (in core)

@Kingdutch I'm actually coming to the conclusion though that we can have both world in a way with your suggestion. I like the 'mental' coupling of var and callable, but that could also be achieved with the naming and keep tell the following story:

"I want to do a ::backwardsCompatibleCall on $currentVersion against $deprecatedVersion resulting in $currentCallable and falling back to $deprecatedCallable"

And there is personal flexibility in argument order with named arguments.

I'm a bit at a loss here to to get consensus though.

Oops, will fix later

First off, thanks everyone for the feedback and suggestions, didn't think this would actually end up a pretty active issue :)

In regards of the DeprecationHelper class I think its current form is as good as it gets. I lean towards the current argument setup and think this is good. Especially considering the scope of the change being basically outside of core.

People are free to callback as they wish, but the example will be with the current kind of function design. We will use this helper in update bot to probably also do some missing 9.x => 10 deprecations (which also need 7.4 support for now), so for now i want to keep it simple. We are still investigating how the different callback designs will affect PHPStan, this will probably be leading in how the callbacks will be applied by Rector. If all is well, when we start building 10 -> 11 pachtes we will consider all options for the callbacks, and try and be as optimal (and readable) as possible.

Hopefully we can angree on the current setup for the main class being good ^^

^{Credit added: @andypost for review and discussion, @heddn for good suggestion and slack, @Charlie ChX Negyesi for an indepth comment on the issue.}

Wasn't set to "Needs review" in #63 but the test is green so assuming this was the goal.

The version that's now in the MR looks good to me and I'd be happy to see it merged.

Wow, tons of action in here since I last looked. 😅

Mostly still very happy with the state of the MR. My only lingering concerns come from this now being a Component, and intended to be used outside of Drupal(?):

1. Do we want the -dev normalization to have semi-Drupal-specific logic?
2. Similarly, the docs talk about "contributed and custom modules", but if this is a Component, maybe that language should be even more general with something like "a call site" or "calling code" or something.

Maybe this should be remain core utility, and not a component, after all. But I don't feel that strongly on either point, and am happy for committers to decide.

Thanks, everyone!

I think this is OK as a component - while the -dev handling does the opposite of what composer does, I don't think it could happen differently here, and the fact composer has -dev handling means that the concept in general is not a Drupalism. Whether anyone ever uses our components outside core is hard to find out, but if we don't put stuff there they definitely can't.

Committed/pushed to 11.x (10.2.x), thanks!

An issue here is that contrib won't be able to adopt this until support for 10.1 is dropped which is in approximately 9 months. Given this is 100% self-contained, do we want to consider an exceptional exception and backporting it to 10.1? That would mean that contrib modules using it would need to declare a dependency on > 10.1.4 or whichever release it goes out in, so I don't know if that would really help or not. But leaving open for discussion.

If we really made use of the components, contrib could require the 10.2.x version of the component by itself, but... let's not get into that.

+1 to backports. Core itself doesn’t call this, so it can’t possibly be a disruptive change to include it. It can only help the contrib ecosystem, not harm it, even if you need to require at least 10.1.4 or whatever to use it.

Thanks!
-Derek

Yeah if possible to backport to earlier version that would be great, and would make the timing for starting to use this in rector a lot easier since this means we can start using it asap to create rules for Drupal 10 -> 11.

The fact is that this is a completly isolated class from core, so there is no risk to degrade.

So as i asked earlier, a backport would be something the community would most probably benefit from with almost no downside to the stability of core.

+1 to backporting as it's self contained and can help contrib.

Also published the changed record, on 10.2.0 for now but will need updating if we backport.

@xjm also +1d this in slack, but said we should add it to the release notes for the next 10.1 patch release. Adding tags and marking needs work for that bit.

Thank you! 😊

Added a short release note, linking to the change record.

Releasenote looks great. Setting RTBC

Anything that is blocking or soft-blocking for the major version compatibility tooling can be major, IMO, and a task.

Cherry-picked to 10.1.x. Thanks!

Maybe a dumb question: Why not back-porting this helper in all Drupal 9 versions (even not supported)? What exactly would be the risk? I really need something like this NOW, when I port contribs from 9 to 10

Adding to release highlights for 10.2.0 to make a bigger splash about this :)