Tests fail, and the patch introduces more than 30 coding standards errors.
"I don't see any test failure on lower version of Drupal"
Yes currently that was true. But if you make all the changes proposed in this issue, it breaks Rules for core < 10.3.
"Regarding un unexplained changes: There are following two test which are failing on D10.3 and hence those changes are made"
Please open a separate [10.3] issue for those test failures!
I have opened other issues for the D10 deprecations noted above - please contribute patches in those issues.
I don't see anything in here that is D11 specific, other than the direct dependency changes, which are done wrong anyway because this module can't be D9 and D11 compatible at the same time, and things like the typed_data dependency need their versions changed as well.
I will fix the dependencies when all the D10 test issues have been resolved - that way we can test against D11 and see what real issues we have rather than just declaring this module D11 compatible even though it fails.
Again, this is why I didn't want an issue like this open. It is FAR MORE work to sort through all these unrelated changes and open new issues myself. So again, since there doesn't seem to be anything D11 specific here, and it's not serving as a tracking issue, and the specific issues raised are not being moved to their own issues like I requested, I am closing this issue again.
I just finished getting typed_data working in D11, which is a prerequisite for Rules, and there are other very specific issues that need to be solved before Rules can be ready for D11. In particular 📌 [10.3] Update ConditionManager to support PHP Attributes Active . Please help with those.
TR → created an issue.
MR testing is turned on for D11 now. All green.
Leaving this issue open because I have to review the dependencies and releases for this branch and for 2.0.x and 8.x-1.x.
MR testing is now turned on for D11 and it shows that this module is now currently working in D11.
None of the casts in the above MR are needed.
Most of the changes were D10 changes, not D11.
Regardless, nothing left to do here.
Thanks, looks good. I made some changes to the comments and merged.
In case it helps anyone else, the attached patch got me out of this hole while things get sorted. Basically it just puts the annotations back into the core request_path plugin, allowing the attributes to carry on working whilst Rules can do its thing.
Yes, that should work if it's just request_path that's not being found on your site.
That's what core should have done in the first place - left all those annotations in place. They are just code comments after all so they don't hurt anything. And they would have ensured that the core plugins in 10.3 remained compatible with code written for 10.2.
By removing the annotations and replacing them with attributes, core is forcing contribute to switch to attributes immediately when going from 10.2 to 10.3, in a minor-point transition.
That's part of a larger issue being addressed in
📌
Integrate Typed Data Widgets
Needs review
There are work-arounds linked in that issue, if you really need them.
Two things:
- Don't need the conditional because 4.0.x is going to have a minimum of Drupal 10.3. (The requirements haven't been changed yet in composer.json because, with the other 10.3 problem, I don't want to people switching to 4.0.x yet until I know it's working in 10.3).
- You're not using the new class correctly. See the change notice at RecursiveExtensionFilterIterator is deprecated → for details.
The first issue is because of the core Drupal change made in RecursiveExtensionFilterIterator is deprecated →
The second one is a duplicate of your issue in 🐛 [10.2] PHPUnit errors: non-existent service plugin.manager.field.field_type_category Needs review , and there is a fix in that issue.
I have changed the issue title to reflect that the first one is the only one that needs to be addressed here.
Need a new branch to support the D10.3 changes. The 8.x-3.x branch will be marked as conflicting with D10.3.
All cspell-flagged words have been corrected or marked as ignore, as appropriate.
I am currently experimenting with that in the scheduled pipelines, and will make the changes to .gitlab-ci.yml when I have a workable set of tests.
The "before" test shows this:
CSpell: Files checked: 409, Issues found: 65 in 30 files.
The number of unrecognised/misspelled words is 41Ironically, unrecognized is misspelled in the error message ...
Many of these are deliberate - either for test purposes or as variable names or in files that shouldn't be parsed. But this serves as a baseline for the issue.
Tests show PHPCS running green again.
This change requires Drupal 10.3+, so this MR also changes the core requirements for the 4.0.x branch.
I have verified, by re-testing in GitLabCI against D10.3, that the MR does fix the failures of the kind You have requested a non-existent service "plugin.manager.field.field_type_category"
There are still other failures, so the tests don't currently run green.
I think all that this needs now is to rebase the MR against 4.0.x and remove the dependency changes - those will be handled in another issue. I will be doing this myself later tonight.
The 8.x-3.x branch will not be supporting D10.3+
Going forward, the new 4.0.x branch is dropping support for D9 and requiring D10.3+, and will provide support for D11.
So this issue is only going to be addressed in 4.0.x.
Working on getting the branch configured and the tests running now ...
TR → created an issue. See original summary → .
Added related issue for the dependency changes.
I opened up a new issue for the validateBy() change. See 📌 [11] Symfony deprecations in Constraint plugins Active
@Anaconda777
@ressa
This issue, and this issue queue, is not about ECA.
Please don't hijack issues for your own agenda.
This was opened as a Drupal 7 issue.
Rules already has a .gitlab-ci.yml in in 8.x-3.x branch.
I think it's still true that you can't do D7 testing on GitLabCI. But if this has changed please provide a D7 patch to do that.
So the issue comes from extending one of core's plugin managers and overriding the protected getDiscovery method. The backwards compatibility policy does not include php classes (except base classes) nor protected methods.
Really, you can't extend a core plugin type without implementing PluginManagerBase::getDiscovery() (or in this case, overriding DefaultPluginManager::getDiscovery()). If you don't implement discovery, you can't find the annotation/attribute classes that define your plugin extensions - you will only get the default core annotation/attribute classes and thus can not add or modify any of these defining properties of plugins.
So to fall back on "not core's problem because the discovery method is protected" is a bit of a cop out. Either Drupal is supposed to be extensible or it isn't. And if it is, then any function needed by subclasses for this extension is fair game. If it wasn't supposed to be overridden then it should have been marked private.
Regardless, this core change from annotations to attributes could have very easily been done in a manner that DIDN'T break contrib. Specifically, in order to be backwards compatible, ALL core plugins should have retained their annotation. Annotation appears in the docblock of the plugin, so if the plugin manager is looking for attributes then it will still find attributes and ignore the annotation. But if the plugin manager (or overridden plugin manager) doesn't have the D10.3 changes yet, it would still work because it would still find the annotations. Then the annotations could have been dropped in D11.
Putting all the BC burden into the core plugin manager implementations only works if the plugin managers are never extended.
And while adding attributes to the core plugins doesn't break BC, removing the annotations from those core plugins DOES. I suppose you could say that since annotations are just code comments, removing them is perfectly allowed between minor point versions, but really it was not necessary and wouldn't have broken anything to leave them in there. And would have avoided this issue.
Can you use DeprecationHelper::backwardsCompatibleCall in the getDiscovery method and do it the new way for 10.3+ and the old way for 10.1 and 10.2? See https://mglaman.dev/blog/writing-backward-compatible-deprecation-fixes-c...
You'd need to bump the minimum version to 10.1, which is the minimum supported version now anyway.
The fix is perfectly do-able. But it does involve extending the core Attributes classes to support Rules extensions for the Condition plugins.
With DrupalCI, we had forward-looking tests which would have revealed this problem as soon as core made the change. But DrupalCI is being turned off and won't test higher than D10.2. And GitLabCI by default runs only against the current core version, so the D10.3 errors didn't start showing up until 10.3 became the current version a few days ago.
Now that I know about the problem, it's not too hard to fix, but it will take more than a day.
Changing dependencies in composer.json and/or rules.info.yml won't prevent this - it's occurring because existing sites are upgrading Drupal core, and they're not picking up a new version of Rules when that happens. New dependencies would only help people trying to install Rules on 10.3.
It's apparent that I have to make a new branch anyway, what with dropping D9 support and with the 10.2/10.3 break (which affects typed_data as well, which Rules depends on).
"Same problem as described, for me it was "user_role" like described in #7 above. My solution was to export all my "reaction rules" and save each locally"
For those of you who want to do that, type drush help rules:export where it shows you how to exports all rules and save them individually with just one easy command.
Also need to tell cspell to ignore phpcs.xml.dist, since core doesn't do that by default apparently.
Yeah, turns out api.drupal.org has some very outdated documentation. Drupal core was changed 3 months ago to add that type hint to the parent class, but that still doesn't show in the documentation on api.drupal.org.
For more details: https://drupal.slack.com/archives/C220WV2TW/p1719173216196289
The core commit that changed this for D11 was 📌 Symfony deprecations in Constraint plugins Fixed
I opened 📌 [10.3] New method getLabel() on FieldItemDataDefinition Needs review for an issue that fixes a core change made in Drupal 10.3. That is not part of making this module D11 compliant.
Patch taken from @deepakkm in 📌 Drupal 11 release Active
As an example of what I'm asking:
- public function validatedBy() {
+ public function validatedBy(): string {Why is this done? It's not wrong to add a return value type hint, but it's not required for D11.
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Validatio...
If return type hints are to be added then IMO they should be done all at once, per class, rather than method-by-method one at a time. That's what I have done in the past, because adding type hints has the possibility of breaking code that extends these classes and also may require a minimum PHP version, thus breaking sites running under a lower PHP version.
- $assert->fieldValueEquals('data[value]', $context_definition->getDefaultValue());
+ $assert->fieldValueEquals('data[value]', (string) $context_definition->getDefaultValue());Likewise here. The context definition object explicitly defines data types for each value, and the return of getDefaultValue() will always be the correct data type. Why does the return need to be cast? If it is to satisfy static analysis, then I would say we shouldn't do this. If the return value is actually wrong and needs to be cast for the test to work, then there's a problem elsewhere that needs to be solved rather than just having the test coerce the type.
As a quick way to recover, renaming rules/src/RulesServiceProvider.php should stop the error on the site. May have to do the usual clear your caches and rebuild the service container with drush. Be aware that this will break Rules and may cause other errors on your site if you have Rules enabled and active Rules in use.
Drupal locates service providers by the naming pattern, so renaming this file to literally any other name will prevent Drupal from finding it. This service provider is how the core Drupal ConditionManager gets replaced by the Rules version, and renaming it will cause Drupal to use the default core ConditionManager.
It's troubling that Drupal introduced a major BC breaking change between minor point releases of core. 10.3 is not backwards compatible with code that works under 10.2. The fix, from my point of view, is a new branch that supports 10.3 and up. All the plugins in Rules and its dependency TypedData need* to be rewritten for PHP attributes. All the plugins in modules that support and extend Rules need to be rewritten for 10.3. And that's a LOT of plugins. About 100 in Rules/TypedData alone. The only way IMO to manage this BC break is to have separate branches for 10.2 and 10.3.
*"need" is a strong word, but to do the same thing as core in an attempt (and fail) to support both the old and the new plugin discovery schemes is clearly not going to work. The way to avoid similar failures in the future is to provide a clean break by having different branches support the different schemes.
8.x-1.0 will not be ported to D11. It is for legacy support only. Thus, MR11 is not needed and won't be used.
MR10 is almost all out of scope - most of the changes have nothing to D11 compatibility. Other than changing the version dependencies. And none of the changes are explained in this issue.
For coding standards changes, please open a new issue. Likewise for other changes not related to D11 compatibility.
Also, most of MR10 is adding back Context classes that were deliberately removed from 2.0.x. That should not be done. Read the deprecation notices in the code you're trying to add - those notices explicitly say the code is intended to be removed from 2.0.x