Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Currently if you define a container element that is empty, you get empty markup. This may or may not be desirable, but currently you cannot define this behavior.
This was pointed out on #2892304: Introduce footer region to ContentEntityForm
Proposed resolution
Implement #optional property for the Container render element, in the same manner as Details.
Remaining tasks
Do it
Needs Followup, See #41, #42 and #43.
User interface changes
None
API changes
New method added: Container:preRenderContainer
.
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#32 | actions-optional-property-do-not-test.patch | 764 bytes | Manuel Garcia |
#32 | 2893586-32.patch | 5.51 KB | Manuel Garcia |
#32 | interdiff.txt | 1.33 KB | Manuel Garcia |
#29 | 2893586-29.patch | 6.25 KB | Manuel Garcia |
#29 | interdiff.txt | 1.52 KB | Manuel Garcia |
Comments
Comment #2
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedComment #4
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedOk clearly i got too excited. Let's try this again...
Comment #5
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedLet's not abuse the
preRenderGroup()
method and just add a newpreRenderContainer()
one like\Drupal\Core\Render\Element\Details::preRenderDetails()
.And we also need test coverage :)
Comment #6
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedComment #7
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedThanks @amateescu for the review, all good points.
Here is a patch addressing #5 and adding a WIP render test.
I believe the test cases are ok, but the test itself is not correct... I only seem to get the result of rendering what's inside the container, but not the wrapping divs around it, any clues how to do this properly? Unfortunately there is no test coverage for
Details::preRenderDetails
to have a look at.Comment #9
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedAlright I can't figure that one out, so taking a different route now by defining a new BrowserTest instead.
I'm getting a weird
InvalidArgumentException: Invalid database prefix: in Drupal\Core\Test\TestDatabase->__construct() (line 81 of /var/www/drupal8/core/lib/Drupal/Core/Test/TestDatabase.php).
error locally on all tests making use ofform_test
module, hoping it's just me, let's see what the bot says...Comment #10
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedComment #12
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedOK at least its only me with the errors. Hopefuly this will come back green.
Any clue why somone would get this error?
Comment #14
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedOK this is embarrassing... let's try it again.
Comment #16
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedOK the failure seems correct, so it looks like we need to have a look at
preRenderContainer
.That said I cannot work on this until I can actually run the test locally (see #12), so essentially I'm blocked.
Comment #17
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedOK did a bit of thinking, and decided to try this,
Container
is not the same asDetails
, so I think doing it like this makes more sense since you can just add child elements directly under the container in the array.Also, please keep in mind that I am still unable to run this test locally, so I have to rely on the bot's feedback :)
Comment #19
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedPerhaps #17 wasn't such a good idea... can't really do much else here sorry, but we should continue from #14.
Comment #20
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedActually, the logic in #17 is spot on! The only problem is that the test is missing the point of "visible" children:
\Drupal\Core\Render\Element::getVisibleChildren()
does not refer to elements which print some markup or not, it is about visible render arrays.You can see the test coverage from
\Drupal\Tests\Core\Render\ElementTest::testVisibleChildren()
and\Drupal\Tests\Core\Render\ElementTest::providerVisibleChildren()
about what exactly is a visible render element :)Comment #21
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedOH I get it... also had a look at
Element::isVisibleElement
, which helped finally clear this out for me.So a visible element has nothing to do with whether or not when you render the element it actually produces any markup. A visible element is just an element who's
#type
is nothidden
,value
nortoken
, and that if it has an#access
property, it is (or returns)TRUE
.Thanks so much @amateescu for helping out on this, much appreciated!
I think we're good to go with this now :D
Comment #22
dawehnerNitpick: Let's document its an array.
This introduces
'#optional'
as a new feature, so let's add a change record, but also maybe add the default value to\Drupal\Core\Render\Element\Container::getInfo
? On top of that I'm wondering whether we should skip if there is any value already set in'#printed'
Note: You can store the result of assertSession() in a variable. I've seen @larowlan requesting it before.
\Drupal\Core\Render\Element\Actions::getInfo
inherits from container, so should it inherit this pre_render function as well?Comment #23
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedThanks @dawehner for the review!
#22.1 Done
#22.2 All makes sense to me, Code changes done, change record coming soon.
#22.3 Done
#22.4 Don't see why not, done.
Comment #25
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedMissing comma... oops!
Added CR: https://www.drupal.org/node/2895127 :)
Comment #27
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedIt's good to have tests. Fixing logic introduced in #23...
Comment #28
vijaycs85probably rephrase to say "should render" instead of "should not"?
Now makes me wonder why don't we have '#printed' check on details (i.e. core/lib/Drupal/Core/Render/Element/Details.php:87)
probably add child like normal do? i.e. $form['container_name']['child_1_name']?
Manuel Garcia++ for extensive test coverage.
Comment #29
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedThanks @vijaycs85 for the review!
Re #28:
#printed
prevents unneeded calls toElement::getVisibleChildren()
, so we should do that inDetails
as well. Probably doesn't occur too often but every little bit counts I suppose. Either way, not in scope here =)Comment #30
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedFWIW, I don't agree that we should also bring the Actions element in the scope of this issue.
Missed a doxygen spot here :P
Comment #32
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedYeah I guess the changes to Actions is of out of scope, and probably very rarely this would have a use-case there. We can always add it later if people want it. Removing those changes here.
I also attach a patch which we could use should we ever want to add the optional property to Actions.
Fixing also #30.2 here.
Comment #33
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedUpdated the CR removing references to the Actions element.
https://www.drupal.org/node/2895127
Comment #35
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedPing...
Sorry to bring this up, but it is sort of blocking #2892304: Introduce footer region to ContentEntityForm, which in turn is making us repeat unnecessary code for published check-boxes (like on #2834546: UI for publishing/unpublishing block_content blocks)... :S
Comment #36
vijaycs85Thanks @Manuel Garcia. Looks like the review comments in #28 and #30 are addressed and patch is looking good. Let's get this in!
Comment #37
lauriiiIt took me a long time to understand this issue because of the issue summary was quite confusing. For anyone revisiting this issue, reading the change record and comments #21 and #22 helped me understand the change better.
Committed cff5bad and pushed to 8.5.x. Thanks!
Comment #39
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedApologies @lauriii - I should've updated the description here after #21, thanks for the extra effort in understanding the issue!
Comment #40
Wim LeersThis actually won't work correctly in all cases: if the sole child is a placeholder, then it is impossible to know when the container is being rendered whether it will be empty.
See #953034: [meta] Themes improperly check renderable arrays when determining visibility.
Was this considered? Should the change record and docs for
#optional
mention this?Comment #41
Berdir#optional already exists for e.g details elements, I don't see why we have to solve this here. The 90% use case for this is imho within forms and conditional/access-controlled form elements
Comment #42
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedYup details probably suffers from the same bug... should we be patching
Element::isVisibleElement
to solve it globally? Not sure if that would tackle the problem with the themes that wim mentions. Either way, I don't think we should be fixing this here.Comment #51
quietone CreditAttribution: quietone at PreviousNext commentedComments #41 and #42 ask for the issue in #41 to be solved in a another issue. Adding tag for a follow up and setting to NW.
Comment #52
quietone CreditAttribution: quietone at PreviousNext commentedAdded a followup, #3279600: Add support for a sole child placeholder to the Container render element.
This issue was committed to 8.5.x, restoring the version and returning to fixed.
Thanks.