Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Status: Active » Needs review
FileSize
22.17 KB

Status: Needs review » Needs work

The last submitted patch, 2: entity-manager-listener-3028656-2.patch, failed testing. View results

Berdir’s picture

Status: Needs work » Needs review
FileSize
26.97 KB

Missed a few.

martin107’s picture

Status: Needs review » Reviewed & tested by the community

a) The idea behind this issue is good.

b) After a visual scan of the patch - nothing extraneous, all code changes have a clear intent, everything implemented well

c) There are a couple of modifications, for example ModuleInstaller.php that make use of \Drupal::service() where I would like to see direct injections. But after a little reflections I can why this is the best thing to do as there is a @todo notices marking the code as soon to be changes by xyz and so the larger code block will soon be refactored away.

Berdir’s picture

Thanks for the review. Note this will conflict with #3023981: Add @trigger_error() to deprecated EntityManager->EntityRepository methods and I'd prefer for that to get in first as it unblocks other issues.

DI for ModuleHandler is very complex and it's not visible from the patch context, but $entity_manager isn't injected either. So using DI there is IMHO not in scope of this issue.

  • alexpott committed 28bf970 on 8.7.x
    Issue #3028656 by Berdir, martin107: Add @trigger_error() to deprecated...

Berdir credited alexpott.

Berdir’s picture

Status: Reviewed & tested by the community » Fixed

Looks like this was committed, didn't conflict after all :)

Status: Fixed » Closed (fixed)

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