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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martin107 created an issue. See original summary.

TR’s picture

Just 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.

martin107’s picture

just 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

martin107’s picture

Status: Postponed » Needs review

Core 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.

martin107’s picture

Status: Needs review » Needs work

The last submitted patch, 5: 3045738-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

martin107’s picture

Status: Needs work » Postponed

Core 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.

martin107’s picture

Status: Postponed » Needs review

Drupal 8.8.x has worked out its issues .. I am reopening, retesting.

TR’s picture

Note: 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!

Status: Needs review » Needs work

The last submitted patch, 5: 3045738-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

TR’s picture

Status: Needs work » Needs review

The 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:

  1. Do we really need DeprecatedServicePropertyTrait in RulesIntegrationTestBase ? I know we don't in RulesEntityIntegrationTestBase - there's an "unused use" coding standards violation about that change ...
  2. Is there any way to make these changes in Rules without failing on core <8.8.x ?

I suspect we have to defer these changes until 8.7.x becomes unsupported, which is in about 1 year?

martin107’s picture

Thanks for working out the php 7.2 / 7.3 serialization thing .... I had not figured that out.

11.1)

Do we really need DeprecatedServicePropertyTrait

No that was a failed experiment of mine... deleted.

11.2)

Is there any way to make these changes in Rules without failing on core <8.8.x ?

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.

I suspect we have to defer these changes until 8.7.x becomes unsupported, which is in about 1 year?

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 ]

Status: Needs review » Needs work

The last submitted patch, 12: 3045738-12.patch, failed testing. View results

martin107’s picture

Status: Needs work » Needs review
TR’s picture

I 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:

Status: Needs review » Needs work

The last submitted patch, 15: 3045738-15.patch, failed testing. View results

TR’s picture

Status: Needs work » Needs review
martin107’s picture

Thanks 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..

jonathan1055’s picture

Adding #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

TR’s picture

Title: Convert entity manager service usage to split up services ( Part 2 ) » Convert entity manager service usage to split up services ( Part 2 ) when Drupal 8.8 becomes the lowest supported version
Status: Needs review » Postponed
jonathan1055’s picture

TR’s picture

I 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.

jonathan1055’s picture

Just 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.

TR’s picture

Would we have expected these to be removed with the patch?

Yes.

jonathan1055’s picture

I 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.

TR’s picture

That 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?

TR’s picture

#26 was bad so I cancelled the test. I forgot to git add everything.

jonathan1055’s picture

I 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:

$this->drupalGet('admin/config/workflow/rules');
$this->clickLinkByHref('disable/ruleid')
$this->clickLinkByHref('enable/ruleid')
$this->pressButton('Delete')

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.

TR’s picture

I 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.

jonathan1055’s picture

Issue summary: View changes

Updated issue summary as those deprecation warnings were not fixed in this issue, but in #3114966: Remove legacy usage of entity.manager instead.

TR’s picture

Status: Postponed » Needs review
TR’s picture

I retested #29 and it still applies.

TR’s picture

  • TR committed 59076ce on 8.x-3.x authored by martin107
    Issue #3045738 by martin107, TR, jonathan1055: Convert entity manager...
TR’s picture

Status: Needs review » Fixed

Committed. Glad we can finally be done with this entity manager mess.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.