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

Date field required marker Array

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

philipz’s picture

Issue summary: View changes
plopesc’s picture

Status: Active » Needs review
FileSize
679 bytes

It 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.

philipz’s picture

Status: Needs review » Reviewed & tested by the community

This was an easy one. Patch applies and looks like problem is fixed, thanks!

plopesc’s picture

Status: Reviewed & tested by the community » Needs work

Back to Needs work after some lines on IRC with @Cottser.

I'll try to dig more on this issue later.

star-szr’s picture

If 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!

plopesc’s picture

Status: Needs work » Needs review
FileSize
564 bytes

Hello,
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:

Twig_Error_Syntax: The text to be translated with "trans" can only contain references to simple variables in "core/modules/datetime/templates/datetime-wrapper.html.twig" at line 20 in Drupal\Core\Template\TwigTransTokenParser->checkTransString() (line 105 of /home/plopesc/projects/drupal8/core/lib/Drupal/Core/Template/TwigTransTokenParser.php).

But, splitting it into three lines:

{% trans %}
  {{ title|passthrough }}
{% endtrans %}{{ required|passthrough }}

It works fine, not sure if I'm missing something or it is a possible twig bug.

Any thoughts??

Regards.

swentel’s picture

Component: field system » datetime.module

Moving to the right component

star-szr’s picture

Assigned: Unassigned » markhalliwell
Status: Needs review » Needs work
Issue tags: +Twig

Thanks for investigating this @plopesc!

If you look back at the original call to t() (pre-Twig), it looked like this:

$output .= '<h4 class="label">' . t('!title!required', array('!title' => $element['#title'], '!required' => $required)) . '</h4>';

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:

{{ '!title!required'|t({ '!title': title, '!required': required }) }}

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.

markhalliwell’s picture

Assigned: markhalliwell » Unassigned

Yes, 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.

markhalliwell’s picture

FWIW though, the patch in #2, is probably the better route.

plopesc’s picture

Status: Needs work » Needs review
FileSize
686 bytes

Re-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 that

<span class="form-required" aria-hidden="true">*</span>

is 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.

markhalliwell’s picture

That 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.

<span class="form-required" aria-hidden="true">*Required</span>

or to hide the text until it's hovered over:

<span class="form-required" aria-hidden="true">*<span class="text">Required</span></span>
plopesc’s picture

Yeah, 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 :)

plopesc’s picture

vijaycs85’s picture

Here is the patch that does same fix with less number of code. either #11 or this patch, +1 to RTBC.

star-szr’s picture

Adding 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().

  $variables['messages'] = array(
    '#theme' => 'form_required_marker',
    '#element' => $element,
    '#access' => !empty($element['#required']),
  );

If we stick with this way…

+++ b/core/modules/datetime/datetime.module
@@ -226,13 +226,8 @@ function template_preprocess_datetime_wrapper(&$variables) {
+  $variables['required'] = !empty($element['#required']) ? drupal_render($form_required_marker) : NULL;

This should default to '' (empty string) instead of NULL.

Status: Needs review » Needs work

The last submitted patch, 15: 2160365-datetime-required-15.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
888 bytes
758 bytes

Thanks, updating NULL with empty string.

star-szr’s picture

Created 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.

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

As 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?

plopesc’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.02 KB
1.9 KB
1.75 KB

Adding 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! :)

The last submitted patch, 21: datetime_required_render-2160365-21-test-only.patch, failed testing.

star-szr’s picture

Status: Needs review » Needs work
+++ b/core/modules/datetime/datetime.module
@@ -227,13 +227,11 @@ function template_preprocess_datetime_wrapper(&$variables) {
+  $form_required_marker = array(
+    '#theme' => 'form_required_marker',
+    '#element' => $element
+  );

Thanks @plopesc! We should add a trailing comma after $element per https://drupal.org/coding-standards#array.

plopesc’s picture

Status: Needs work » Needs review
FileSize
1.9 KB
1.13 KB

Oh! Sorry @Cottser, slap me please ;)

Comma added

Status: Needs review » Needs work

The last submitted patch, 24: datetime_required_render-2160365-24.patch, failed testing.

plopesc’s picture

star-szr’s picture

Status: Needs work » Reviewed & tested by the community

Thanks! :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0f0edea and pushed to 8.x. Thanks!

  • Commit 0f0edea on 8.x by alexpott:
    Issue #2160365 by plopesc, vijaycs85: Date field required marker...
xjm’s picture

Title: Date field required marker rendered as "Array" » [HEAD BROKEN] Date field required marker rendered as "Array"
Priority: Normal » Critical
Status: Fixed » Reviewed & tested by the community

This broke HEAD.

* Drupal\datetime\Tests\DateTimeFieldTest (142 pass(es), 1 fail(s), and 0 exception(s))
   - [fail] [Other] "Required markup found" in DateTimeFieldTest.php on line 101 of Drupal\datetime\Tests\DateTimeFieldTest->testDateField().

To revert:
git revert 0f0edea

webchick’s picture

Title: [HEAD BROKEN] Date field required marker rendered as "Array" » Date field required marker rendered as "Array"
Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs work

Reverted.

  • Commit 959fa9d on 8.x by webchick:
    Revert "Issue #2160365 by plopesc, vijaycs85: Date field required marker...
vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.9 KB
938 bytes

The test fail is an impact of #2226665: Only set "add more" wrapper on fields with cardinality >1 (committed at 1e04c67). Here is the fix.

Status: Needs review » Needs work

The last submitted patch, 33: 2160365-diff-24-33.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review

diff file with wrong extension. Setting 'Needs review' for first patch in #33

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Great, thanks @vijaycs85!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

2160365-diff-24-33.patch no longer applies.

error: patch failed: core/modules/datetime/lib/Drupal/datetime/Tests/DateTimeFieldTest.php:98
error: core/modules/datetime/lib/Drupal/datetime/Tests/DateTimeFieldTest.php: patch does not apply

vijaycs85’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll

@alexpott, that's diff file with wrong extension. Can we use 2160365-datetime-required-33.patch in #33 please?

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed e088b8f and pushed to 8.x. Thanks!

  • Commit e088b8f on 8.x by alexpott:
    Issue #2160365 by plopesc, vijaycs85: Date field required marker...

Status: Fixed » Closed (fixed)

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