Core has several test traits which help with setup, e.g.
- Drupal\Tests\media\Traits\MediaTypeCreationTrait
- Drupal\Tests\node\Traits\NodeCreationTrait
- Drupal\Tests\image\Kernel\ImageFieldCreationTrait
These generally follow the pattern of ending in 'CreationTrait'. This helps developers identify them as reusable in their own tests, and automated tools such as Module Builder can find them and suggest them when generating test classes.
EntityReferenceTestTrait is a reusable trait for creating entity reference fields, but it does not follow this pattern.
Rename the trait to EntityReferenceFieldCreationTrait.
Determine whether a deprecation notice is necessary for the existing trait or whether it can just be removed.
| Comment | File | Size | Author |
|---|
Show commandsStart 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:
changes, plain diff MR !5412
Comments
Comment #2
pradhumanjain2311 commentedRenamed the EntityReferenceTestTrait to EntityReferenceFieldCreationTrait.
Comment #3
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue.
While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)
Comment #6
sourabhjainComment #7
smustgrave commentedThis issue was tagged novice for new users @sourabhjain based on your posts you can work on non novice issues.
Reroll into MR seems fine.
Comment #8
xjmThanks @sourabhjain. Note that converting a patch to a merge request without other contributions to the issue does not receive credit. Also, I wonder if maybe you could work on some more advanced contributions rather than novice ones? Based on your profile, you are ready for that. 🙂 It's best to leave novice-tagged issues for those who are new to the contribution process.
Comment #9
xjmSaving credits, including for @smustgrave for mentoring.
Comment #10
xjmAlso remember to hide old patch files when converting an issue to an MR.
Comment #11
xjmThanks everyone for your work on this.
In general, we treat test base classes and traits as APIs, since it's very likely that contrib might also use them in their tests. It's okay to change them in a minor release as development code, but we should have a change record for it.
Thanks!
Comment #12
xjmComment #13
smustgrave commentedNot sure if CR falls under novice but wrote a quick one https://www.drupal.org/node/3401941
Comment #14
bramdriesenI don't think there is more to say in the CR :) looks good @smustgrave!
If the issue is novice, the CR could be novice as well. But in general I don't think creating a CR is a novice task as it sometimes requires deep understanding of what is being fixed.
Comment #17
xjmI reviewed locally with
git diff --color-wordsand verified that the only changes are renaming the trait. I also grepped forEntityReferenceTestTraitand confirmed there are no remaining usages.Committed to 11.x and 10.2.x as a minor-only test API improvement and internal API change. Also published the CR. Thanks!
Comment #19
sourabhjainHi @xjm @smustgrave
I intentionally deferred work on these issues. All the updates were made on November 14, 2023, and I've been addressing them sequentially in the Drupal issue queue without initially considering the tags assigned to each. I acknowledge this oversight and apologize for any confusion. Moving forward, I will ensure to prioritize and address issues based on their respective tags.
Comment #20
sourabhjain.
Comment #21
mglamanSo, contrib modules cannot run tests on 10.0, 10.1 if they fix this for 10.2. Or they cannot run for 10.2. There is no bridge.
Can the CR be updated to explain how to use class aliasing to make this work? Or have Drupal core provide a class alias for the trait.
Comment #22
berdirI think this should not have been committed and should be reverted. The old trait should be deprecated and I'm honestly not sure if that's worth that.
Comment #23
berdirFollow-up: #3403491: Rename EntityReferenceTestTrait breaks multi-version testing