Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The original issue had this mission statement
Search in all of Rules where we use the entity manager and replace it.
This issue, part 2, is about fixing test classes.
In part 1 work was done on test classes after the commit that fixed issues in production code.
I am just transfering that patch here so that we have a distinct issue number. (useful for recovering the commit history [git blame ] )
See #3045525: EntityDeriver: deprecate entityManager. and #3042545: Update last injected entity managers
Comment | File | Size | Author |
---|---|---|---|
#29 | 3045738-29.patch | 4.01 KB | TR |
Comments
Comment #2
TR CreditAttribution: TR commentedJust to be clear, this only applies to Rules unit tests where we currently need to mock the EntityManager because Drupal core still uses the deprecated EntityManager.
The non-test Rules codebase was fixed and the deprecated service was removed three and a half years ago in #2622982: Convert entity manager service usage to split up services.
Drupal core still has hundreds of uses of the deprecated entity.manager service, and that needs to be fixed before we can remove the last of it in our test cases.
Thank you for working on this @martin107 - I hope you can get some results with that core issue.
Comment #3
martin107 CreditAttribution: martin107 as a volunteer commentedjust a little housekeeping
the issue we are postponed on was closed as a duplicate... we are waiting on
https://www.drupal.org/project/drupal/issues/3042545
Comment #4
martin107 CreditAttribution: martin107 as a volunteer commentedCore has resolved issues associated with the entity deriver ( yay drupalcon ) [ on the 8.8.x-dev branch ]
Just triggering testbot so we can see where we now stand.
Comment #5
martin107 CreditAttribution: martin107 as a volunteer commentedReroll.
Comment #7
martin107 CreditAttribution: martin107 as a volunteer commentedCore needs to fix some issues still outstanding ( they did not all get fixed by the core issue I mentioned in #4. )
So I have reopened a core issue -- to fix that.
So putting this issue back in the deep freezer.
Comment #8
martin107 CreditAttribution: martin107 as a volunteer commentedDrupal 8.8.x has worked out its issues .. I am reopening, retesting.
Comment #9
TR CreditAttribution: TR commentedNote: PHP 7.3 tests are currently failing with the serialization bug that affects a lot of Drupal. See #3036623: Notice: unserialize(): Error at offset 8126 of 22729 bytes in Drupal\Component\Serialization\PhpSerialize::decode() for details.
I retested your patch with PHP 7.2/Drupal 8.8 and it runs green!
Comment #11
TR CreditAttribution: TR commentedThe PHP 7.2 / Drupal 8.8.x tests work with this patch! That's good news, and great job getting that patch into core!
But with Drupal 8.7.x, the tests fail worse than before...
So two questions:
I suspect we have to defer these changes until 8.7.x becomes unsupported, which is in about 1 year?
Comment #12
martin107 CreditAttribution: martin107 as a volunteer commentedThanks for working out the php 7.2 / 7.3 serialization thing .... I had not figured that out.
11.1)
No that was a failed experiment of mine... deleted.
11.2)
No the issue in core that makes a difference was cherry picked and committed to 8.7 - it broke head and then was rapidly reverted.
Just by way of conversation - in the polar opposite of what we have here..
Say a large enterprise with a big coding team and many customer spread of a large slow moving code base
then all changes go into the master branch.
Then there is assessment, based on the number of sub project using old versions, about what to cherry pick and backport.
While we are in beta, and some ways off release I would have thought the the beta would target Drupal core 8.8.x
and then as we move out of beta - and demand reveals itself we would back-port as required.
If the release takes longer than expected .. we maybe don't have to do the backport ... I am new here so forgive my ignorance. Do we have a constituency of people using the beta in production ?
[ A justifiable rigidity like that exists with the flag module .. where the beta is semi frozen ]
Comment #14
martin107 CreditAttribution: martin107 as a volunteer commentedComment #15
TR CreditAttribution: TR commentedI agree with how it *should* work, but Drupal hasn't figured out how to do versioning with contributed modules yet. Core is now making API-breaking changes between minor core releases, and we don't yet have a good way to synchronize and associate contributed module releases with the version of core they need. So we have to be safe as much as possible and ensure that the latest Rules release will work with all supported versions of core.
I will be making an -alpha5 release of Rules very soon, and that release should be made to work with Drupal 8.6, which is the current supported version. So -alpha5 cannot have this patch included. Drupal 8.7 will become the current supported version in a few weeks, and Drupal 8.8 will become the current supported version in ~6 months. At that point I should be able to make a new release of Rules which contains this patch.
Here is a re-roll of #12 with the DeprecatedServicePropertyTrait removed:
Comment #17
TR CreditAttribution: TR commentedComment #18
martin107 CreditAttribution: martin107 as a volunteer commentedThanks for the brain dump that was a good thing for me to read.
That perspective is better than mine... I guess I have been getting away with it on the drupal modules as I use them..
Comment #19
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedAdding #3089502: [meta] Rules deprecated code as a related issue as cannot have two parents. Also updated issue summary with the deprecation messages and link to CR
Comment #20
TR CreditAttribution: TR commentedComment #21
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedUpdated I.S. with list of 12x deprecation locations.
Swapped over parent and related.
Comment #22
TR CreditAttribution: TR commentedI re-tested the patch in #15 it it no longer applies due to the many changes we've made to Rules in the past 10 months. Here's a re-roll - just a minor tweak to make sure it still applies and is ready to commit when we need it.
Comment #23
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedJust tried patch #22 on my Travis build test, and on core 8.8 and 8.9 we still get the full set of
EntityManagerInterface::getStorage() is deprecated in drupal:8.0.0 ...
warnings. Would we have expected these to be removed with the patch? I was assuming so, but obviously wrong. Sorry for the noise if this has been explained elsewhere, but it needs to laid out here if we are to undertand and implement this patch once core 8.7 no longer needs to be supported.Comment #24
TR CreditAttribution: TR commentedYes.
Comment #25
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedI was just writing this reply when you posted.
Having looked at the patch, I can see that it only makes changes to two unit/integration test files, so it is clear that it cannot affect any of the functional tests. So the fixes that this patch addresses are not actually displayed by any @trigger_error message at the moment, and we do not currently have any patch for 8.8+ which does address those messages shown. So there is more work to do here than just wait and commit this when 8.7 is no longer supported.
Comment #26
TR CreditAttribution: TR commentedThat seems to be my fault - an old entity.manager usage got committed a few weeks ago as part of the new list builder code, which had a long and tortured history. I had changed all but one usage to the entity_type.manager.
Can you try this new patch and see if the deprecation messages disappear?
Comment #27
TR CreditAttribution: TR commented#26 was bad so I cancelled the test. I forgot to git add everything.
Comment #28
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedI had started to do a bit of analysis on which parts of the tests caused the deprecation, and it was always on a line with one of:
etc, and the result of these is always the main config/workflow/rules list page. So that matches up exactly with what you have said.
And yes, the new patch removes all 28 of the "EntityManagerInterface::getStorage() is deprecated" message. The functional tests all run OK at 8.7, 8.8 and 8.9 with that patch, so you could commit that one change to RulesReactionListBuilder.php now. Then the patch #22 would remain here, waiting for 8.8 to become the lowest supported version.
Comment #29
TR CreditAttribution: TR commentedI opened up #3114966: Remove legacy usage of entity.manager to fix the list builder.
#22 isn't sufficient, because #27 also included a new hunk which removed an unused $entityManager variable. So here is a new patch that is #27 without the one-line fix from #3114966: Remove legacy usage of entity.manager.
Comment #30
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedUpdated issue summary as those deprecation warnings were not fixed in this issue, but in #3114966: Remove legacy usage of entity.manager instead.
Comment #31
TR CreditAttribution: TR commentedComment #32
TR CreditAttribution: TR commentedI retested #29 and it still applies.
Comment #33
TR CreditAttribution: TR commentedComment #35
TR CreditAttribution: TR commentedCommitted. Glad we can finally be done with this entity manager mess.