Problem/Motivation

Discovered via #3581155: Investigate if shipmonk/dead-code-detector would be useful to us

Drupal\Tests\system\Functional\Entity\Traits\EntityDefinitionTestTrait refers to $this->entityDefinitionUpdateManager but it is not declared in the trait.

It also refers to 

$this->state</code, but it is not declared in the trait either.

<h4 id="summary-steps-reproduce">Steps to reproduce</h4>


<h3 id="summary-proposed-resolution">Proposed resolution</h3>

Define <code>$entityDefinitionUpdateManager

in the trait.
Also define $state in the trait.
Remove the property definition from all implementations of the trait.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#8 3581403-nr-bot_i4diq7_8.txt98 bytesneeds-review-queue-bot

Issue fork drupal-3581403

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review

longwave opened merge request !15205

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

The trait also uses $state so let's declare that too.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs change record

May be overkill but could we get a simple CR for this change?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Yeah, just checked contrib and this might break a small number of tests, e.g. https://git.drupalcode.org/project/map_widget/-/blob/510e4352a80ce469832...

mstrelan’s picture

mstrelan commented
Title: Move $entityDefinitionUpdateManager definition to EntityDefinitionTestTrait » Move $entityDefinitionUpdateManager and $state definition to EntityDefinitionTestTrait
Status: Needs work » Needs review
Issue tags: -Needs change record

Added a CR, add $state to the title.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3581403-nr-bot_i4diq7_8.txt98 bytes

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

mstrelan’s picture

mstrelan commented

Bot took 10 mins to flick from NR to NW, that's speedy. I think I messed up the rebase anyway, so leaving NW as I'm out of time.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

OK that's better now

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

CR reads well. Assuming this would be a 12 only? Or would it make it in 11.5?

godotislate’s picture

godotislate
he/him
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

I think there's one \Drupal::service usage in the trait that needs to be updated.

I also updated the IS.

godotislate’s picture

godotislate
he/him
commented

Assuming this would be a 12 only? Or would it make it in 11.5?

There is a type declaration added to $state in base class core/tests/Drupal/KernelTests/Core/Entity/EntityKernelTestBase.php, so I think it'd be 12 only? Will confirm.

smustgrave’s picture

smustgrave commented

Applied the suggestion, will wait to put back in RTBC per #13 in case there's more.

smustgrave’s picture

smustgrave commented

Reverted the change as it broke a bunch of tests.

godotislate’s picture

godotislate
he/him
commented

OK, I assume there must have been a container rebuild involved somewhere.

smustgrave’s picture

smustgrave commented
Status: Needs work » Reviewed & tested by the community

Putting back in RTBC, unless work needs to be done for #13.

  • catch committed a6dd75d0 on main 
    task: #3581403 Move  and  definition to EntityDefinitionTestTrait

By:...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main, thanks!

Lots of commit conflicts on 11.x, but I think we can just have this in main so moving to fixed.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

quietone’s picture

quietone commented

Published the change record

Status: Fixed » Closed (fixed)

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