Problem/Motivation
As outlined in #2430673: ContentEntityBase::__construct() lacks documentation there is some uncertainty over whether ContentEntityBase([LANGUAGE_DEFAULT => , 'fr' => ]) would even work. And even if it does, it would definitely be a good idea to get this tested. Especially since the $values data structure is very ... interesting so a proven to be working example is very useful here.
Steps to reproduce
Proposed resolution
Create a test called on `core/tests/Drupal/KernelTests/Core/Entity/EntityTranslationTest.php` called `testEntityConstructor` to test the ability to call ContentEntityBase contructor with several langcodes.
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|
Issue fork drupal-2436209
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:
- 2436209-test-contententitybase-constructor
changes, plain diff MR !13293
Comments
Comment #1
dawehnerAdapting the parent issue
Comment #2
chx commentedOpsie, sorry, thanks for fixing it, too many browser tabs :)
Comment #3
chx commentedHere we are.
Comment #4
chx commentedMoved the count assert one line down.
Comment #5
berdirAs discussed with @chx, one goal here is essentially to provide examples/code documentation for alternative storage backends. Maybe we could actually refer to this somewhere, e.g. on the content entity storage base class?
The test currently doesn't pass in $bundle and $translations to the constructor.
1. The missing bundle only works because entity_test by default has no bundles and uses the $bundle == $entity_type_id fallback. We should instead (or maybe additionally) create a random bundle with entity_test_create_bundle(), including a test field that only exists on that bundle and create values also for that configurable field.
2. Missing $translations can probably be tested by calling getTranslationLanguages(), it should return the same languages as you create.
I think doing those two things should give you fairly good test coverage of those arguments.
Also wondering if we shouldn't merge this back together with #2430673: ContentEntityBase::__construct() lacks documentation. I wouldn't be surprised if this issue would get pushed back due to missing documentation.
Comment #6
chx commentedComment #7
plachLooks good to me, let's try to RTBC this and see what happens ;)
Comment #9
chx commentedComment #10
siva_epari commentedPatch rerolled.
Comment #11
plachThanks
Comment #12
xjmA couple inline comments in this test would be peachy; it's kind of a wall of code at the moment, so somewhat difficult to see exactly what it tests. ("Works with translations" is not specific.)
I can see the case for separating the scope from #2430673: ContentEntityBase::__construct() lacks documentation or for combining the patches; no strong feelings either way.
Comment #26
smustgrave commentedThank you for creating this issue to improve Drupal.
We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.
Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks!
Comment #27
smustgrave commentedActually seems to still be relevant.
Tagging for novice for a reroll and issue summary update. Both a new user should be able to achieve.
Thanks all
Comment #31
luismagr commentedI think I've re rolled the patch correctly, but I had to modify certain code that has been deprecated during these years.
Please, have a look to it and review if something else is missing.
Thanks
Comment #32
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.
Comment #33
luismagr commentedComment #34
smustgrave commentedStill appears to need an IS update please.
Comment #35
luismagr commentedComment #36
luismagr commentedThanks @smustgrave. I updated the issue summary. However, I'm not sure if I'm missing something for the description.
Again, thanks for the time.
Comment #37
smustgrave commentedBelieve this one is ready to go.
Comment #38
catchCommitted/pushed to 11.x, thanks!