Closed (fixed)
Project:
Rules
Version:
8.x-3.x-dev
Component:
Rules Core
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
22 Oct 2019 at 20:00 UTC
Updated:
11 Jan 2022 at 10:57 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
jonathan1055 commentedHere's a patch for the increase. There is no point in testing this on drupal.org as the .travis.yml file will have no impact.
I understand that @TR the maintainer does not use Travis testing, and that's fine. But this patch will assist me (and possibly others) when testing patches and developing fixes, because it allows the Travis tests to pass. Hence a failed test then would really indicate a new failure, which is better than having to ignore this fixable fault. It will also mean we are alerted when new deprecations get triggered.
Jonathan
Comment #3
tr commentedSure, we can increase the number. But this is something that's going to have to be done frequently, because core has declared that deprecations can be added at any time now, so this number can change on a day-by-day basis ...
Of the new deprecations:
Providing context definitions via the "context" key is deprecated in Drupal 8.7.x and will be removed before Drupal 9.0.0. Use the "context_definitions" key instead.This one is the subject of an open issue that explains why we can't get rid of it yet: #3030295: 'context' deprecated in Rules @Condition annotation
Constructing a ContextDefinition object for an entity type is deprecated in Drupal 8.6.0. Use Drupal\Core\Plugin\Context\EntityContextDefinition instead.This is a core change that is really just wrong and will pose existential problems for Rules. I have raised this point before in the core queue, to deaf ears. The whole premise of Rules is that because of the Typed Data API, we can treat all typed data equally. If we need to program huge switch blocks everywhere to do something different for each different datatype, then all the power, simplicity and elegance of reflection-based code is lost. This change means we CAN'T simply get the context for the data type in a type-agnostic way - we have to get the data, determine the data type, then do different things depending on the type of the data. That's just wrong. The root problem is that core Drupal focuses on Entities to the exclusion of everything else, so they keep putting things into the Entity API that really should belong in the Typed Data API and apply to all data types. The sad thing is, it would be EASY to support both ContextDefinition for all types including entities as well as the specialized EntityContextDefinition for only entities, but core has decided that entity contexts are now going to diverge from field contexts and from contexts for all other data types. I frankly don't have the bandwidth to fight this in core - it needs a sit-down discussion with the core maintainers and I am not invited to those meetings. I talked with @fago about this last year, and I think he agreed with me, but he has not been involved with Rules for a long time now and I don't think he has the time to deal with it either (although he would certainly be more successful influencing core...)
Apart from these, all those other deprecations share the same problem. The whole core deprecation scheme coupled with core "semantic versioning" is a huge problem for contrib modules - if a deprecation isn't backported to ALL the currently-supported versions of Drupal, that means contrib CAN'T remove the deprecated usage immediately. The only choices are:
I am sticking with 1) for now because I can't justify abandoning sites that are using Rules on currently-supported versions of core. But also, given the lack of community support for this project, I don't have the time or the motivation to create and maintain many different 8.x-3.x branches in order to support multiple versions of D8 core. I suspect that many other contrib modules will go the other way - just code to the most RECENT API and abandon any existing sites. That trend has been building for years in the Drupal community, and is reinforced by the deprecations testing because that puts a lot of pressure on module maintainers to fix deprecations IMMEDIATELY (even thought that is opposite of what "deprecation" should mean) but I think it makes a mockery of the Drupal community's claim to "support" several version of D8/D9. Why should I pick Drupal for an organization if there is absolutely no long-term-support on any version, and if the only way to keep it running is to aggressively and constantly spend time and money to keep upgrading everything every few months?
If I'm wrong about all of this or if there's a better way to do things in contrib then I would REALLY like to know because right now I don't see any alternative.
I could really use some help here, someone to participate in the core Drupal discussions and address the needs of contrib modules in general and Rules in particular. I already have far too much on my plate just dealing with Rules etc. - it's a full time job just to keep pace with core changes, let alone accomplish anything new in Rules. And I am NOT working full time on this - I'm just a volunteer who is doing this in my spare time (which comes and goes).
Comment #4
tr commentedOh, and to be clear, I DO want to fix things that we can fix right now. It's just that most of those things CAN'T be fixed because of core Drupal and I'm tired of being publicly shamed for not having them fixed yet when it's out of my control. AND all these deprecations that can't be fixed obscure the very few which CAN be fixed, making it MORE difficult to maintain a contrib module ...
I went through the list in the original post and there are two things that are deprecated but give no indication of which core version they are deprecated in (these were apparently designated as deprecated before core decided to attach a core version number to deprecations).
Because this feature was added in 8.6.x, we should be able to make this change in the current version of Rules and still support all supported Drupal core verisions. The change notice mentions a number of classes, but Rules doesn't use any of them. So it's not clear to me what code in Rules is triggering this (or maybe it's core code that we're using ...). If anyone knows what needs to be done to fix these, please open a new issue with details ...
The deprecation notices don't mention it, but looking at the change notices it seems this change was made in core 8.5.x. So we should be able to fix this in the current version of Rules. I have opened a separate issue for this (see #3089549: Fix TempStore deprecations) and I will be posting a patch there shortly.
Again, I would like to fix all of these deprecations, it's just that core doesn't give contrib modules the tools to do this, as explained previously.
Note that once #3089549: Fix TempStore deprecations goes in you'll have to change the deprecations limit again, so maybe we should leave this open until that is committed ...
Comment #5
jonathan1055 commentedThanks TR for giving all this background info. It really helps to explain where we're at.
Yes, that's fine, ignore the patch in #2
Yep, I agree that this is the better of the two choices
I am also a volunteer doing this in my spare time and I will gladly help where I can, but I do not have your level of experience with OO code design and all the background Core knowledge of the structure and relationships of entities etc. I will participate where I can, for example the recent coding standards investigation and fixes.
I know that they can't be fixed yet (I did say that in the issue summary) and I absolutely did not intend to do any public shaming. I am very sorry and apologise if that is how my post came over.
I will test #3089549: Fix TempStore deprecations on Travis and check for the reduction in deprecation messages.
Maybe this issue could be turned into a Meta and have all the deprecation issues as children, then we'd have all the relevant details here in one place. Your explanation of the reasons why we can't fix things now is very good to have here as a reference.
Do you need me to try any further investigations into
167x: The $memory_cache parameter was added in Drupal 8.6.x and will be required in 9.0.0. See https://www.drupal.org/node/2973262or did you exhaust the trace?One final note, we really do appreciate what you are doing for Rules. Please keep going as we are making progress. What you have done in Rules Essentials is great, really helpful UI enhancements in particular. Thanks for that.
Jonathan
Comment #6
tr commentedI totally appreciate that - you're one of the few who helps out in any way with Rules. Rules is used on >25% of all Drupal sites, but pretty much no-one out of that group of 300,000 contributes to Rules. So the fact that you do anything at all makes you an exceptional case, and you've certainly done more than that! Thank you.
I apologize if this came across as critical of you. It was not - it was directed mainly towards core Drupal and the maintainers of core Drupal who think they are doing good by calling out contributed project which are not "D9 ready" - yet at the same time Drupal core does not ALLOW contributed modules to be D9 ready . There is currently no way for a module to support both D9 and D8.7 or D8.6 (both of which are technically "supported" D8 versions) either through one branch or multiple branches. There are many features of project pages and there are many blog (etc.) posts which tout D9 compatibility and imply that module which aren't D9-compatible are either unsupported or mis-managed or poorly-managed, but for a non-trivial module like Rules it's literally impossible to be D9 compatible and D8.7 compatible.
Again, that wasn't directed at you.
Yes please - It's not clear to me why we are getting that deprecation error. If you know why this is happening I would like to know! Since D8.6 is the lowest-supported version of Drupal core, we should be able to fix this without breaking any supported version.
I'm going to leave this open so that we can change the deprecation count whenever necessary ...
Comment #7
tr commentedI opened #3092084: Add $memory_cache parameter to ReactionRuleStorage for that one. I posted a patch that should fix it. Can you test?
Comment #8
kristen polPer a Slack discussion with Gábor Hojtsy regarding usage of D9 tags (Drupal 9, Drupal 9 compatibility, Drupal 9 readiness, etc.), "Drupal 9 compatibility" should be used for contributed projects that need updating and "Drupal 9" was the old tag for D8 issues before the D9 branch was ready. Doing tag cleanup here based on that discussion.
Comment #9
tr commentedI think you posted to the wrong issue - this issue never had any sort of D9 tag, and is just about managing the deprecations messages for things we CAN'T fix at this time.
If you read my posts above, I'm sure you'll understand why I don't want Rules identified with any sort of D9 tag before core Drupal provides the tools to enable non-trivial contributed modules like this to be compatible with both D8 and D9. The whole point of this issue is that we have 10,000 deprecations that CAN'T be fixed until Drupal 9.0 is released.
Comment #10
kristen polThanks for the clarification. I scanned the comments yesterday and there was a discussion about fixing deprecation issues so that's why I tagged it. If fixing this issue isn't needed for D9 support, then I see why you untagged it.
Comment #11
jonathan1055 commentedChanging this to a meta issue, to act as parent for all the separate deprecation issues.
Comment #12
jonathan1055 commentedBetter title for a meta issue. The monitoring of count can still be done here, but the overal issue title should be short and clear. Collecting all the deprecation issues will make it easier to find them, as many cannot be worked on at the moment, as explained above.
Comment #13
jonathan1055 commentedNow fixed:
New deprecations:
Current deprecation total at core 8.8 is 11,999
Comment #14
jonathan1055 commentedFixed:
#3092093: Using the "owner" public property of a TempStore lock is deprecated in Drupal 8.7.0
#3098110: Deprecation: add $defaultTheme to BrowserTestBase
#3098187: Deprecations: Replace non-boolean use of assertTrue() and assertFalse()
Comment #15
tr commented#3030291: ConfigurablePluginInterface is deprecated in core 8.7.x has been committed.
Comment #16
tr commented@jonathan1055: Can you run this again with the patch from #3030295: 'context' deprecated in Rules @Condition annotation to see if that patch solved all the 'context' key issues? That should take it down by more than 6000 and leave us only with the EntityContextDefinition problem.
Comment #17
jonathan1055 commentedHere are the results of running with dev after this commit + also applying patch #10 of #3030295-10: 'context' deprecated in Rules @Condition annotation
Travis build #164.2 - Core 8.7, PHP7
OK (336 tests, 1407 assertions). Remaining deprecation notices (4843)
build #164.4 - Core 8.8, PHP7.3
OK (336 tests, 1362 assertions) Remaining deprecation notices (3223)
build #164.6 - Core 8.9, PHP7.3
Identical to Core 8.8
So it has taken the totals down by a lot, but not fixed all of the 'Providing context definitions via the "context" key'. I have added a txt file showing the tests which still have these.
I have noticed also that core 8.7 has 45 more assertions that core 8.8 and 8.9 (even though they have the same number of tests), and consequently a higher deprecation count. I do not know why this is, but it has been like this for several weeks at least (so is nothing to do with the latest commits).
Comment #18
tr commentedI couldn't find anything I missed in #3030295: 'context' deprecated in Rules @Condition annotation, so my guess is that these addition "context" keys are coming from our Event plugins, which don't use annotation but rather .yml files for discovery. Thus, I think we need to change the "context" key in our *.rules.events.yml files and Event plugin derivers. I will work on that.
Comment #19
tr commentedI patched the events and combined that with the previous patch. Can you try again and see if the patch in #3030295-15: 'context' deprecated in Rules @Condition annotation takes it to zero now?
Comment #20
jonathan1055 commentedYes that has worked. There are no more 'Providing context definitions via the "context" key' messages.
Travis build #165.2 - Core 8.7, PHP7.0
OK (336 tests, 1407 assertions) Remaining deprecation notices (3139)
Travis build #165.3 & .4 - Core 8.8, PHP7.0 & 7.3
OK (336 tests, 1362 assertions) Remaining deprecation notices (2653)
Travis build #165.5 & .6 - Core 8.9, PHP7.0 & 7.3
Identical to core 8.8
Nice work.
Comment #21
tr commentedWhy doesn't the EntityManager deprecation issue show up in the deprecations list? #3045738: Convert entity manager service usage to split up services ( Part 2 ) when Drupal 8.8 becomes the lowest supported version
This is another thing we can't get rid of until D8.8 is the lowest-supported version - see the test output in that issue where it fails miserably for D8.7 because core 8.7 still uses EntityManager.
Comment #22
jonathan1055 commentedUpdated I.S. following the recent commits.
I don't know. I had also spotted that anomoly when checking the deprecations via #3091969-6: Add drupalci.yml file for Rules. I will try to find out.
Comment #23
jonathan1055 commentedIt has just started showing up recently, and there are 12x in 8.8
Also added new issue #3108310: Deprecation: add $defaultTheme to javascript test EventBundleTest
Comment #24
jonathan1055 commentedHere's a patch to use phpstan via drupalci.yml to run a static check for @deprecated code. This is not for committing, but useful to have on this issue.
Comment #25
jonathan1055 commented#3108310: Deprecation: add $defaultTheme to javascript test EventBundleTest is fixed.
PHPstan did not show any deprecations that we were not already aware of, so that's good
Comment #26
jonathan1055 commentedUpdated deprecation counts following addition of new tests from the following:
#2769511: Cache is not invalidated when Rules are changed
#3104328: Move list builders from D8RE into Rules Assigned to: tr
#3108494: Exception when updating cache with empty data
#3110461: Add rules_ban actions and conditions to functional tests
Comment #27
jonathan1055 commentedAs requested in #2769511-27: Cache is not invalidated when Rules are changed here is the travis patch to reflect the new deprecation counts.
Comment #29
tr commentedCommitted #27.
Comment #30
jonathan1055 commentedThe
28x: EntityManagerInterface::getStorage() is deprecated in drupal:8.0.0 and is removed from drupal:9.0.0have been fixed by #3114966: Remove legacy usage of entity.managerIssue summary updated. New patch to reduce the allowed deprecatation count attached.
Comment #31
jonathan1055 commentedDue to increased tests from #3114756: Port node.rules.inc actions to D8 the Travis tests all fail on deprecation count which is masking any new deprecation or actual test failure. Please would you consider commiting this new patch to alter the allowed numbers. Setting this issue status to 'needs review' to alert to the fact that a patch exists, but there is no need to test this on d.o. Thank you.
Comment #33
tr commentedDone.
Comment #34
tr commentedAdded #3137977: Replace assertInternalType() calls with dedicated methods when Drupal 8.8 becomes the lowest supported version
Comment #35
tr commentedComment #36
jonathan1055 commentedHi TR,
Now that we don't need to test at core 8.7 I have updated the .travis.yml file. This will also assist me when testing the upcoming deprecation patchs. I've also added drupalci*.yml to the .gitignore as I know you are not keen on committing a drupalci.yml file, but I need to keep them locally to use in patches when testing for deprecations on drupal.org. Having these ignored by Git will make it easier.
Thanks.
Comment #37
tr commentedDo we even need the .gitignore? I thought best practice was to not have a .gitignore in contrib projects.
Comment #38
jonathan1055 commentedThat could be right, I don't know. Other contribs do, but that's not saying it is right. So a developer can have a global gitignore file which includes '.gitignore' in the list to ignore. I already have this, and can put anything I like in a .gitignore in any contrib folder. The problem was when I came to edit the Rules .gitignore I got the git status message showing me it was changed. I don't know when Rules got that .gitignore file added, but I'm fine if you want to delete it. Here's a patch which does that, plus the change to .travis as before.
Comment #39
tr commentedRules 8.x-3.0-alpha6 has been released, so we can now finish off these deprecations.
@jonathan1055: I guess #38 slipped off my radar - is it ready to go now or does it need to be re-rolled?
Comment #40
jonathan1055 commented\Drupal\Tests\PhpunitCompatibilityTrait:setExpectedException() is deprecated in drupal:8.8.0has been fixed by #3098199: Replace deprecated $this->setExpectedException when Drupal 8.8 becomes the lowest supported versionThe \Drupal\Core\Path\AliasManager class is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0has been fixed by #3101026: Replace deprecated path.alias_manager and AliasManager when Drupal 8.8 becomes the lowest supported versionI've updated the issue summary.
Good that you did not commit patch #38 as I've now increased the allowed deprecations count because we have more tests running. Patch #40 can be committed. Thanks
Comment #42
tr commentedCommitted #40.
Comment #43
jonathan1055 commented#3088489: Path aliases have been converted to revisionable entities, fix when Drupal 8.8 becomes the lowest supported version is fixed and we now have just the one single deprecation message.
Comment #44
tr commentedThis is a pretty terrible solution but it works locally so I'm curious to see whether DrupalCI will like it.
@jonathan1055: Can you check the deprecations with this patch? It fixes them for me ...
Let me repeat, terrible solution, but if it allows us to be D9 compatible then we can add this as a hack to avoid the regression in core while we work on getting core fixed.
Comment #45
jonathan1055 commentedI think this should have its own separate issue, so that we have a proper place to discuss, test, record the commits and not get lost in this general Meta issue. On that new one we can also check on Core progess and the possible/eventual removal of this code from Rules (when ever that may be). Could you open a new issue and put the patch there, please? Thanks and sorry to be a pain, but might as well do it from the start.
Comment #46
tr commented#3161582: EntityContextDefinition breaks the context system
Comment #47
tr commented@jonathan1055: Now that #3161582: EntityContextDefinition breaks the context system is committed, can you update the issue summary? I want to keep this issue open because Drupal core 9.1.x is now deprecating other things, such as #3153803: [Symfony 5] Update EventDispatcher::dispatch() to make it forward-compatible with Symfony 5 which we need to track. I imagine this will be an on-going process as core is continuously evolving - we will always need to cope with supporting the currently-supported versions of Drupal core as well as the newly-emerging version of Drupal core.
Comment #48
jonathan1055 commentedYes, I confirm that after #3161582: EntityContextDefinition breaks the context system we have zero deprecation messages at core 8.8, 8.9 and 9.0. I have removed the reference to #3126747 as this is included in the summary on #3161582
Comment #49
tr commentedComment #50
tr commentedA new D9.1 issue.
Comment #51
jonathan1055 commentedAdded new child issues:
#3172039: [9.1] EventDispatcherInterface::dispatch() parameter argument order changed
#3172046: [10] Fix automated test results - Support for keys without a placeholder prefix is deprecated (Rules)
No immediate hurry to fix these, but good to have the issues created for reference.
Comment #52
jonathan1055 commentedPatch #24 should be permenantly visible at the top of this issue, so we can easily see the current state, and retest when required.
Comment #53
jonathan1055 commentedAdded #3189035: [9.1] UnitTestCase::assertArrayEquals() is deprecated in drupal:9.1.0
Comment #54
kopeboyCan we sponsor Rules on Drupal 9 in any way?! Please
Comment #55
tr commented@kopeboy: Thank you for asking.
Speaking for myself, my biggest constraint is time. So what I need most is more people working on Rules - helping to write documentation and create examples, helping to code - specifically finishing some missing features - and helping to answer posts in the issue queue, including testing and reviewing submitted patches. These are the things that will speed development. If you have some specific areas of interest let me know and I can give you a list of things that I think have the most return for the least effort, or I can give you a list of some of the hardest problems if that's more your style.
Comment #56
tr commentedLet's close this, since it's really all about the D8 -> D9 transition.
Let's start fresh with a new meta for the D9 -> D10 transitions.
Comment #57
kopeboyI'm not a developer so I guess I can help with testing and documentation.
Not much I can document until components can't be used with reaction rules from the UI, so please point me to some new developments I can test from a site builder perspective
Comment #59
jonathan1055 commentedGood idea. I have created #3257972: [meta] Rules deprecated code D9 -> D10 and moved the two remaining open child issues onto it.