- Issue created by @jabeler
- 🇦🇺Australia mstrelan
FWIW the vast majority of Rules usage stats is from Drupal 7.
Sure, but that applies to Drupal core as well since 7.x is still the largest install base. Rules is still a fairly widely used module, and having it installed on 10.3 causes things to crash and burn in spectacular fashion.
Of course, people should always keep backups and test updates on a dev/staging site before upgrading live sites but as you see by the early reports linked above that isn't always happening.
I suppose there is a question around whether core release notes should be responsible for listing all known incompatibilities, and I would say the answer is no...but there could be exceptions.
My sites will continue to run 10.2.x until an update for Rules is available (or a replacement is found) so I'm just trying to help others avoid a ton of frustration here.
Updating release notes is not enough as there is evidence many do not read them. If 10.3 introduced a regression (Did it? Could other modules be affected?) we need an issue to address that. This could become that issue.
10.3 does appear to introduce breaking changes and other modules could certainly be affected.
https://www.drupal.org/project/rules/issues/3454056#comment-15652473 📌 [10.3] Update ConditionManager to support PHP Attributes Active
- 🇦🇺Australia mstrelan
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 → .
That said, there are a fair few contrib modules that seem to be overriding this same method.
- 🇺🇸United States tr Cascadia
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, overridingDefaultPluginManager::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.
- 🇮🇳India nishantkumar155
on configuration import also getting below error on rules.
In DiscoveryTrait.php line 53:
The "entity_bundle:node" plugin does not exist. Valid plugin IDs for Drupal\rules\Core\ConditionManager are: rules_data_comparison, rules_data_is_empty, rules_list_contains, rules_list_count_is, rules_enti
ty_has_field, rules_entity_is_new, rules_entity_is_of_bundle, rules_entity_is_of_type, rules_ip_is_banned, rules_node_is_of_type, rules_node_is_promoted, rules_node_is_published, rules_node_is_sticky, rule
s_path_alias_exists, rules_path_has_alias, rules_text_comparison, rules_entity_field_access, rules_user_has_role, rules_user_is_blocked, webform - 🇬🇧United Kingdom 2dareis2do
I am also getting The "entity_bundle:node" plugin does not exist. on 4.x branch when using rules.
Also I cannot see any reference to rules breaking in the release notes here?
https://www.drupal.org/project/drupal/releases/10.3.0 →
10.3 seems to break quite a lot of things. I think I am going to roll back for now, especially where I have rules dependency.
- 🇬🇧United Kingdom 2dareis2do
It seems a lot of effort has been put into breaking contrib compatibility here.
https://www.drupal.org/node/3229001 →
I tend to agree with TR on this issue. Perhaps it would be better to find a solution that deprecates rather than remove compatibility with popular contrib modules. If core is goinf to make breaking changes, why not do it in a major release?
Looking at the original issue, I am not an expert, but it does seem to me that this change is a little premature. i.e. only one of the 5 supporting tasks looks like it has been closed.
Remove Doctrine from Drupal 11.
- 🇦🇺Australia mstrelan
It seems a lot of effort has been put into breaking contrib compatibility here.
The CR that you've linked to here does not require you to use the attributes, you can use either attributes or annotations. This is in order to maintain contrib compatibility.
I tend to agree with TR on this issue. Perhaps it would be better to find a solution that deprecates rather than remove compatibility with popular contrib modules. If core is going to make breaking changes, why not do it in a major release?
Refer to #9. That said, I'm struggling to see why rules needs to override ::getDiscovery in the first place? The only difference I see is that it's not passing
$this->additionalAnnotationNamespaces
, so why not just make that property and empty array and let the parent class do the work?Remove Doctrine from Drupal 11.
Doctrine annotations won't be removed in Drupal 11, that would be making the issue far worse.
- 🇬🇧United Kingdom 2dareis2do
Thanks for clarifying @mstrelan
The CR that you've linked to here does not require you to use the attributes, you can use either attributes or annotations. This is in order to maintain contrib compatibility.
Just, it says in the title of that CR that these are "Plugins converted from Annotations to Attributes in 10.3.0" - so I assumed that support for Annotations had been dropped, hence the issue with Rules breaking in 10.3. Apologies if I have misunderstood.
TR says:
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.
and you say
I'm struggling to see why rules needs to override ::getDiscovery in the first place? The only difference I see is that it's not passing $this->additionalAnnotationNamespaces, so why not just make that property and empty array and let the parent class do the work?
TBH I don't really have a clue about plugin discovery and really don't know why it is needed either?
Obviously the maintainers didn’t know this would break projects that do their own plug-in discovery.
Can we please just move ahead to fix this?
- 🇺🇸United States tr Cascadia
"Can we please just move ahead to fix this?"
Well, I don't know about all of you, but that's what *I* have been doing all week.Core had like 50 issues and dozens of people working on making these changes in core over a period of time.
I'm having to do the same changes in Rules by myself, for at least as many plugins as core has, and I don't get the luxury of spreading out that work over months. As I said, it will take a little while, but it will get done.
I don't think it's appropriate to change the core release notes. This will be fixed while 10.3 is still less than a month old, then Rules will be back to working with core. Then are you going to go back and change the release notes again? Since releases are supposed to be immutable, I don't see how this could be put in until the next minor release, and as others have said no one reads the release notes anyway until *after* they have had a problem.
- 🇬🇧United Kingdom joachim
The problem is not that Rules overrides the getDiscovery() method. Lots of core plugin managers do that too.
The problem is that Rules swaps the ConditionManager service for its own class:
public function alter(ContainerBuilder $container) { // Overrides the core condition plugin manager service with our own. $definition = $container->getDefinition('plugin.manager.condition'); $definition->setClass(ConditionManager::class); }
and that class changes the discovery:
// Swap out the annotated class discovery used, so we can control the // annotation classes picked. $discovery = new AnnotatedClassDiscovery($this->subdir, $this->namespaces, $this->pluginDefinitionAnnotationName); $this->discovery = new ContainerDerivativeDiscoveryDecorator($discovery);
which means that Condition plugins in core that are since 10.3 defined using attributes aren't getting picked up, since the replacement ConditionManager service isn't using attributes.
- 🇦🇺Australia mstrelan
@joachim that's exactly the method I'm talking about in #14. It should just fall back to the parent implementation.
- 🇺🇸United States tr Cascadia
No, the real problem is that Rules needs to extend the core
ContextDefinition
class. Because@ContextDefinition
is used in the core@Condition
annotation, the only way to force the use of the Rules version ofContextDefinition
for Condition plugins is to override the annotation discovery for Condition plugins so that the Rules extension toContextDefinition
would be picked up.Adding an additional namespace for annotation discovery with
additionalAnnotationNamespaces
is NOT sufficient because the core namespace gets searched first and the core@ContextDefinition
annotation (which uses the coreContextDefinition
class) is picked up first. So no, we can't just "fall back to the parent implementation" because the parent implementation picks up the wrong class. That's why this method was overridden in the first place - if the parent implementation worked there would be no reason to do that, would there?Again, it's all about being able to extend the core
ContextDefinition
. Because this class is not provided by a service, and because the core annotations were not designed for extensibility, Rules had to override how annotation classes were discovered for this plugin type.With Attribute discovery, on the other hand, the
ContextDefinition
class is discovered by the "use" statement in the class that is using the attribute. (In attributes we usenew ContextDefinition()
instead of@ContextDefinition
). So all we have to do to overrideContextDefinition
is to "use" the correct version of the class. As it should be.So Attributes are easy. Switching 100% to attributes, without maintaining a BC layer, will make it possible for Rules to drop the custom discovery, but this was not possible with annotations.
Unfortunately, it is still not yet possible to drop the custom discovery in Rules. To maintain a BC layer in Rules, we now have to support THREE discovery methods:
- AnnotatedClassDiscovery
- AttributeClassDiscovery
- AttributeDiscoveryWithAnnotations
The last one doesn't just call out to the first two - it's a copy of the first two together, tied with some logic.
Thus, Rules still has to override
AnnotatedClassDiscovery
, but it now ALSO has to overrideAttributeDiscoveryWithAnnotations
. But since core madeAttributeDiscoveryWithAnnotations::getAnnotationReader()
a private method in 10.3 (WHY?), it can't be overridden. Instead, Rules now also needs its own copy ofAttributeDiscoveryWithAnnotations
just to override this one method for backwards compatibility. And this will now be fragile and subject to future breakage if core modifies that class.Regardless, if you're really interested in the details, it's all there in the Rules issue queue. I just don't appreciate the attitude that this should have been an easy one-line fix, or that Rules is to blame because Rules should have done something differently from the start. There's a lot of back-seat driving going on here.
Core developers apparently knew for a very long time that this change would cause major disruption ( 🌱 [policy, no patch] Allow both annotations and attributes in Drupal 11 Active ), but depreciation warning were never added ( 📌 Triggering deprecations for plugins using annotations when core plugin type has been converted to attributes Needs work ) and, most importantly, this change was made BETWEEN MINOR POINT RELEASES OF CORE. And by DELETING the annotations present in the core Condition plugins - a step that was not necessary for this transition to attributes - core broke backwards compatibility with contrib code like Rules that relied on those annotations.
It's too late to avoid the problem, but looking back is useful so that we learn from this so as to possibly avoid something similar in the future. To summarize then, what happened was:
- Major API and architectural change between Minor releases of core which disproportionately affected plugin-heavy contributed modules like Rules.
- No deprecation warnings in 10.2 before the change or 10.3 after the change, so problem didn't show up in testing before 10.3 broke things, and didn't even show up AFTER 10.3 broke things.
- Unnecessary removal of annotation in core plugins between minor releases of core, when leaving them in would have prevented this issue.
This was coupled with the switch from Drupal CI to GitLab CI at the same time, with Drupal CI not being able to test against 10.3 and the default settings of GitLab CI not testing against 10.3 until the day 10.3 was released. That made solving this problem a lot more difficult, because a lot of testing issues needed to be addressed first before making these huge changes in the plugins that are a fundamental part of Rules and the Rules ecosystem. Being more proactive about the switch between Drupal CI and GitLab CI is one thing I could have done differently, but last fall when I first switched Rules testing to the new system it was too early - we were not able to do D7 testing in GitLab CI and there was no easy way to do matrix testing (in fact, it was being discouraged at the time), so I abandoned my efforts back then until Drupal testing on GitLab CI was more mature. And it remained a low priority for me as long as the tests were working. I could have gotten back to that earlier this year, and perhaps avoided this mess, but frankly anyone else in the Drupal community could also have contributed a patch to the Rules .gitlab-ci.yml to help out.
- Status changed to Fixed
5 months ago 9:18pm 25 July 2024 - 🇺🇸United States tr Cascadia
Rules has been fixed. 📌 [10.3] Update ConditionManager to support PHP Attributes Active
The new version of Rules, 4.0.x, fixes this problem as of 21 July. A fixed-point release 4.0.0 was made today after a few days of testing and waiting to see if any further problems arose.
4.0.x works with Drupal core 10.3+ and Drupal 11.
Rules 8.x-3.x still exists and still runs properly on versions of Drupal 10.2 and lower.
When you upgrade core using composer, your version of Rules will be upgraded to 4.0.0, and your version of Typed Data (which Rules depends on, and which also needed substantial changes) will be upgraded to 2.1.0.
So as far as I'm concerned, that closes this issue - there's nothing left to do here and no need to change any release notes.
I don't know what the appropriate issue status is ... I chose "Fixed" as a way to get attention of anyone following this, instead of "Closed (xxx)" which would immediately hide the issue.
- 🇬🇧United Kingdom catch
Core developers apparently knew for a very long time that this change would cause major disruption (#3400121: [policy, no patch] Allow both annotations and attributes in Drupal 11), but depreciation warning were never added (#3265945: Triggering deprecations for plugins using annotations when core plugin type has been converted to attributes) and, most importantly, this change was made BETWEEN MINOR POINT RELEASES OF CORE.
This is not a fair representation of how this change is being made.
We know that it will be disruptive, eventually all plugin implementations will need to convert from annotations to attributes. It's a big change.
However that is exactly the reason we aren't issuing deprecations yet. We decided that core would need to be fully converted before we issue deprecations for contrib plugins still using annotations.
This isn't an oversight, it's an attempt to reduce disruption. Adding the ability to issue deprecations would not have flushed out the issue in rules earlier because it's about a completely different thing (the plugin implementations themselves, not the plugin managers). Eventually we will need to deprecate annotation discovery altogether, that will be a further step after enabling plugin managers to issue their own deprecations. Again, doing that now would be premature and more disruptive.
Every single API addition (which is what this is, until we start triggering deprecations), is made in minor releases of core. We don't make any api additons in major releases of core because that would be more disruptive - completely breaking compatibility like Drupal used to in the 6/7/8 transitions. Putting MINOR in block capitals does not make a normal thing more scandalous.
In this case an API addition to core turned out to result in breaking a contrib module. The comments here about how rules does things are not 'blaming' rules, but pointing out that it does something unusual. If we'd known about the rules breakage before 10.3.0 we possibly could have done something - maybe leaving annotations alongside attributes, maybe something else. However it's impossible to predict every way that every change will affect every contrib module.
As you point out, gitlab makes it easier to test with multiple versions of core, so should help to find issues like this earlier in the future, i.e. add that level of prediction that we currently don't have.
Automatically closed - issue fixed for 2 weeks with no activity.