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
Date field required marker is rendered as "Array". There was an issue that converted datetime.module theme_ functions to Twig which might be responsible for that:
#1963476: datetime.module - Convert theme_ functions to Twig
Proposed resolution
Not sure if this needs fixing in datetime.modules or somewhere else in field/theming.
Remaining tasks
Write patch, review, commit.
User interface changes
Fixes wrong required marker rendering.
API changes
None
Comment | File | Size | Author |
---|---|---|---|
#33 | 2160365-diff-24-33.patch | 938 bytes | vijaycs85 |
#33 | 2160365-datetime-required-33.patch | 1.9 KB | vijaycs85 |
#24 | intediff.txt | 1.13 KB | plopesc |
#24 | datetime_required_render-2160365-24.patch | 1.9 KB | plopesc |
#21 | interdiff.txt | 1.75 KB | plopesc |
Comments
Comment #1
philipz CreditAttribution: philipz commentedComment #2
plopescIt lloks like t does not work because
template_preprocess_datetime_wrapper
is passing the required variable as a render array instead of the plain HTML.Calling to render before and passing the HTML to the twig template, it works fine.
Regards.
Comment #3
philipz CreditAttribution: philipz commentedThis was an easy one. Patch applies and looks like problem is fixed, thanks!
Comment #4
plopescBack to Needs work after some lines on IRC with @Cottser.
I'll try to dig more on this issue later.
Comment #5
star-szrIf this is an appropriate fix (would be nice to avoid the early rendering) then drupal_render() should be used as mentioned. Thanks for your work on this @plopesc!
Comment #6
plopescHello,
After more digging, I found that render array are not converted into HTML strings when are inside {% trans %} tags. Given taht the required span is not translatable, IMHO does not make sense put it inside that tag.
So, putting it out of the tag, but in the same line:
{% trans %}{{ title|passthrough }}{% endtrans %}
the page throws an error:
But, splitting it into three lines:
It works fine, not sure if I'm missing something or it is a possible twig bug.
Any thoughts??
Regards.
Comment #7
swentel CreditAttribution: swentel commentedMoving to the right component
Comment #8
star-szrThanks for investigating this @plopesc!
If you look back at the original call to t() (pre-Twig), it looked like this:
Via #1963476: datetime.module - Convert theme_ functions to Twig.
So we can't change the end call to t(), that would be a string change and definitely out of scope for fixing this issue. Maybe we need to pre-render render arrays for the trans tag, we could do that using a set tag. Or we could go back to using a t filter and the array syntax, before #2047227: Update templates to leverage support for the Twig {% trans %} tag extension landed:
Or trans tags maybe need to be able to render renderable arrays on their own, or alternatively have better error handling for when they contain render arrays. I think it would be better to throw an exception instead of printing Array. Assigning to @Mark Carver to get an opinion on how trans tags should work/be used in this case.
Comment #9
markhalliwellYes, use the
|t
filter instead. As the trans tag mentioned:The text to be translated with "trans" can only contain references to simple variables
That indeed means that only simple variables, not complex ones (like arrays) can be used. We didn't see much use for putting in costly support for render arrays when 9/10 we're translating text only. This is also why we left the
|t
filter in, to deal with cases like this.Comment #10
markhalliwellFWIW though, the patch in #2, is probably the better route.
Comment #11
plopescRe-rolling patch in #2, using drupal_render() instead of render() as pointed @Cottser in #5.
About the inclussion of
required
variable in trans tag, I believe it does not have any sense, given thatis not a translatable string, but as you said, maybe it is out of the scope of this issue. But this is my opinion :)
Thank you very much for your help.
Comment #12
markhalliwellThat is why
!required
is part of the locale string, not every site wants just a simple asterisk. I have seen several sites change it to include text, which of course would need to be translated.or to hide the text until it's hovered over:
Comment #13
plopescYeah, but I believe it should be translated at the level of
theme_form_required_marker
overwriting to keep consistency.Moreover, not sure if the result of this theme function is translated in other places in the code.
Anyway, we agree this is out of the scope of this issue.
Regards :)
Comment #14
plopesc11: datetime_required_render-2160365-11.patch queued for re-testing.
Comment #15
vijaycs85Here is the patch that does same fix with less number of code. either #11 or this patch, +1 to RTBC.
Comment #16
star-szrAdding a related issue since this may be changing.
We could also consider using #access, keeping in mind that it's an extra function call to drupal_render().
If we stick with this way…
This should default to '' (empty string) instead of NULL.
Comment #18
vijaycs85Thanks, updating NULL with empty string.
Comment #19
star-szrCreated a follow-up/related issue to add better exception handling for these cases (#2201813: Twig trans block should throw an exception when printing a render array) but it's hard to say that should hold this up. It would be nice to add tests here but at the moment I'm not sure what that would look like other than checking the exact output.
Comment #20
star-szrAs much as I'd like to RTBC this, can we please add a simpletest assertion somewhere to verify that the date field required marker is displayed properly?
Comment #21
plopescAdding basic test for this feature and splitting the
$form_required_marker
array definition in order to follow coding standards.I think now it should be ready to go! :)
Comment #23
star-szrThanks @plopesc! We should add a trailing comma after $element per https://drupal.org/coding-standards#array.
Comment #24
plopescOh! Sorry @Cottser, slap me please ;)
Comma added
Comment #26
plopesc24: datetime_required_render-2160365-24.patch queued for re-testing.
Comment #27
star-szrThanks! :)
Comment #28
alexpottCommitted 0f0edea and pushed to 8.x. Thanks!
Comment #30
xjmThis broke HEAD.
To revert:
git revert 0f0edea
Comment #31
webchickReverted.
Comment #33
vijaycs85The test fail is an impact of #2226665: Only set "add more" wrapper on fields with cardinality >1 (committed at 1e04c67). Here is the fix.
Comment #35
vijaycs85diff file with wrong extension. Setting 'Needs review' for first patch in #33
Comment #36
star-szrGreat, thanks @vijaycs85!
Comment #37
alexpott2160365-diff-24-33.patch no longer applies.
Comment #38
vijaycs85@alexpott, that's diff file with wrong extension. Can we use 2160365-datetime-required-33.patch in #33 please?
Comment #39
alexpottCommitted e088b8f and pushed to 8.x. Thanks!