Support from Acquia helps fund testing for Drupal Acquia logo

Comments

stefan.r created an issue. See original summary.

stefan.r’s picture

joelpittet’s picture

The context of why we need to cast these keys is lost without being part of the original issue, I think maybe leave this one in the parent issue, no?

stefan.r’s picture

@joelpittet my worry is the parent issue is complex enough as it is, while this one is easily reviewable, limited in scope and would have 0 effect if we commit it (as we're just casting strings to string).

The only thing we'd need to prove is that these array keys may hold output from t(), and find a good way to work around that.

But yes, unless the parent is bumped to critical this one likely won't go in without the parent going in as well... but we can always merge this back in there as soon as we're happy with the changes here?

stefan.r’s picture

stefan.r’s picture

Issue summary: View changes
FileSize
211.73 KB
271.78 KB

/wrong issue

joelpittet’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -945,7 +945,7 @@ public function getEntityTypeLabels($group = FALSE) {
-        $options[$definition->getGroupLabel()][$entity_type_id] = $definition->getLabel();
+        $options[(string) $definition->getGroupLabel()][$entity_type_id] = $definition->getLabel();

@@ -959,7 +959,7 @@ public function getEntityTypeLabels($group = FALSE) {
       // Make sure that the 'Content' group is situated at the top.
-      $content = $this->t('Content', array(), array('context' => 'Entity type group'));
+      $content = (string) $this->t('Content', array(), array('context' => 'Entity type group'));
       $options = array($content => $options[$content]) + $options;

Maybe this set of options doesn't need to be using translated strings for it's keys?

Kind of like Tasks but using better keys like run it through one of those *Identifier helpers.

Using a translation for a key on options seems problematic for UI related things.

stefan.r’s picture

Definitely yes! The issue title may have been a bit too specific, I'd rather rewrite the code where possible so as to not do the string cast if it doesn't break APIs

lauriii’s picture

Lets see how hard this fails..

stefan.r’s picture

Actually none of this will work :( the keys in options arrays are for optgroups, which do have to remain translated.

Sadly I think casting to string is our only option here, if we want to maintain BC.

Interestingly this is not documented in the Form API docs...

Status: Needs review » Needs work

The last submitted patch, 9: cast_array_keys_that-2561259-9.patch, failed testing.

stefan.r’s picture

Screenshot of what this would regress to:

On the bright side, it makes the patch very reviewable -- it's just just Html::getClass() and all the rest are optgroup keys, where a cast to string is the only choice if we want to keep the label translated and we want to keep the render array.

Likely most of the disruption for contrib with objects as array keys will be around these optgroup keys considering they are 90%+ of the conversions in core as well.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
13.72 KB
stefan.r’s picture

I've given this a final review and I think this all looks pretty straightforward now...

  1. +++ b/core/modules/views_ui/admin.inc
    @@ -92,8 +92,9 @@ function views_ui_add_ajax_trigger(&$wrapping_element, $trigger_key, $refresh_pa
    +  $button_title = !empty($triggering_element['#title']) ? (string) $triggering_element['#title'] : $trigger_key;
    

    Technically this could have been refactored not to use the string casts, ie by using SplObjectStorage, but why bother when a string cast is just as easy?

  2. +++ b/core/modules/ckeditor/ckeditor.admin.inc
    @@ -113,6 +117,9 @@ function template_preprocess_ckeditor_settings_toolbar(&$variables) {
    +      $group_name = (string) $group_name;
    

    Same here.

Status: Needs review » Needs work

The last submitted patch, 13: 2561259-13.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review

Unrelated failure in BlockContextMappingUpdateFilledTest

stefan.r queued 13: 2561259-13.patch for re-testing.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

I think its a little bit sad that we have to do this (cast t functions all over the place) but I don't see there is other way than this if we are going to make t function return translation wrapper because there is no way that array keys would support objects. I tried to grep array keys using t function in during #9 so I'm hoping they are all included.

stefan.r’s picture

I have posted a CR draft for the parent issue at https://www.drupal.org/node/2564451 based on the conversion effort in core. I think it's just array keys (treated in this issue), strpos and tests that pose a problem.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 2561259-13.patch, failed testing.

stefan.r’s picture

Status: Needs work » Reviewed & tested by the community

Seemingly unrelated testbot fail:

Drupal\system\Tests\Update\RouterIndexOptimizationTest 346 4 0

Message Group Filename Line Function Status
GET http://ec2-52-88-182-40.us-west-2.compute.amazonaws.com/checkout/update.... returned 0 (0 bytes). Browser UpdatePathTestBase.php 246 Drupal\system\Tests\Update\UpdatePathTestBase->runUpdates()
After all updates ran, entity schema is up to date. Other UpdatePathTestBase.php 263 Drupal\system\Tests\Update\UpdatePathTestBase->runUpdates()
GET http://ec2-52-88-182-40.us-west-2.compute.amazonaws.com/checkout/update.... returned 0 (0 bytes). Browser UpdatePathTestBase.php 246 Drupal\system\Tests\Update\UpdatePathTestBase->runUpdates()
After all updates ran, entity schema is up to date.

stefan.r queued 13: 2561259-13.patch for re-testing.

alexpott’s picture

I think optgroups might be a big problem :(

What happens if they contain HTML? Hmmm... does that even make sense. I just they never should because this is a select list thing - right?

stefan.r’s picture

This will likely be the most frequent "problem" in contrib if t() outputs an object, luckily it won't really be a problem at all as all they'll have to do is a simple string cast, per https://developer.mozilla.org/fr/docs/Web/HTML/Element/Optgroup the string in the array key will end up in the label attribute of the optgroup tag... which can't contain markup :)

<select>
  <!-- when #options = [(string) t('Groupe 1') => ['Option 1.1']] -->
  <optgroup label="Groupe 1">
    <option>Option 1.1</option>
  </optgroup> 
  <optgroup label="Groupe 2">
    <option>Option 2.1</option>
    <option>Option 2.2</option>
  </optgroup>
  </optgroup>
</select>
stefan.r’s picture

@catch mentioned #342316: Introduce proper Form API #types for 'option' and 'optgroup', and make #options consistent. may be a nice issue to work on so that in the CR we won't have to recommend casting optgroup array keys to string (which is otherwise perfectly fine as they're plain text so it shouldn't block this patch)

alexpott’s picture

+++ b/core/lib/Drupal/Component/Utility/Html.php
@@ -47,13 +47,17 @@ class Html {
+    // Cast to string in case $class is a TranslationWrapper.
+    if ($class instanceof SafeStringInterface) {
+      $class = (string) $class;
+    }

Where are the t()'d things that are causing this?

stefan.r’s picture

#26: we don't need it anymore - I took it out and the TranslationWrapper tests still pass.

stefan.r’s picture

Title: Cast array keys that may hold translated strings to string » Cast (optgroup) array keys that may hold translated strings to string

Considering this is 95% about optgroups it may be worth mentioning that in the title

joelpittet’s picture

Wrapping lines at 80 chars.

Still RTBC, IMO. The comment would be nice to avoid having to remove it later in the parent issue(we always forget).

lauriii’s picture

RTBC++

dawehner’s picture

+++ b/core/modules/locale/src/Form/ImportForm.php
index 82bb51d..7c26932 100644
--- a/core/modules/views/src/Plugin/views/area/TokenizeAreaPluginBase.php

--- a/core/modules/views/src/Plugin/views/area/TokenizeAreaPluginBase.php
+++ b/core/modules/views/src/Plugin/views/area/TokenizeAreaPluginBase.php

Note: this conflicts with a critical also fixing it.

stefan.r’s picture

@dawehner - so let's fix whichever goes in later in a reroll or on commit? What's the issue so I can keep an eye on it?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 29: cast_optgroup_array-2561259-29.patch, failed testing.

stefan.r’s picture

Seemingly unrelated fail in UserTokenReplaceTest.php

andypost’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -945,7 +945,10 @@ public function getEntityTypeLabels($group = FALSE) {
    +        $options[(string) $definition->getGroupLabel()][$entity_type_id] = $definition->getLabel();
    
    +++ b/core/modules/locale/src/Form/ImportForm.php
    @@ -94,8 +94,10 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +        (string) $this->t('Existing languages') => $existing_languages,
    +        (string) $this->t('Languages not yet added') => $this->languageManager->getStandardLanguageListWithoutConfigured(),
    
    +++ b/core/modules/views/src/Plugin/views/area/TokenizeAreaPluginBase.php
    @@ -52,14 +52,18 @@ public function tokenForm(&$form, FormStateInterface $form_state) {
    +    $optgroup_fields = (string) t('Fields');
    ...
    +      $options[$optgroup_fields]["[$field]"] = $handler->adminLabel();
    
    +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
    @@ -862,19 +862,23 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    +      $options[$optgroup_fields]["{{ {$this->options['id']} }}"] = substr(strrchr($this->adminLabel(), ":"), 2 );
    ...
    +        $options[$optgroup_arguments]['%' . ++$count] = $this->t('@argument title', array('@argument' => $handler->adminLabel()));
    +        $options[$optgroup_arguments]['!' . $count] = $this->t('@argument input', array('@argument' => $handler->adminLabel()));
    

    so key ($optgroup_fields) get cast early but label casted at render time

    this could lead to inconsistency of render between group and label when default interface langcode is different from entity language

  2. +++ b/core/modules/views_ui/admin.inc
    @@ -92,8 +92,9 @@ function views_ui_add_ajax_trigger(&$wrapping_element, $trigger_key, $refresh_pa
    +  $button_title = !empty($triggering_element['#title']) ? (string) $triggering_element['#title'] : $trigger_key;
    

    empty() should be done on string, in case of wrapper better to check for interface

stefan.r’s picture

@andypost do we have any proof 1 could ever happen? IMO they should always both be shown in the interface language - if they dont, that sounds like a separate bug?

stefan.r’s picture

Status: Needs work » Needs review
andypost’s picture

stefan.r’s picture

Thanks @andypost for raising the issue... if this is actually a problem it seems separate and unrelated to the current patch though, as: t('foo') === (string) t('foo') === (string) new TranslationWrapper('foo')

andypost’s picture

stefan.r’s picture

cool. As to the empty check - I wonder if new TranslationWrapper('') should even be possible. To me that should always return an empty string (such as with SafeString).

So I think we're good there? If the object is set, it'll be non-empty and we go ahead...

stefan.r’s picture

This only left RTBC state because of an unrelated test fail - #36 had further feedback but this shouldnt hold this up. Anyone willing to give this a final review?

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

I think both comments in #36 has been addressed.

amateescu’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -945,7 +945,10 @@ public function getEntityTypeLabels($group = FALSE) {
+        // We cast the optgroup label to string as array keys must not be
+        // objects and t() may return a TranslationWrapper once issue #2557113
+        // lands.

The "once issue #... lands" part of this comment (and all its duplicates) looks really weird. Why do we need to mentioned something that may or may not happen in the future?

stefan.r’s picture

@amateescu the plan was to take it out once it gets in (or not), but I do agree, can you suggest better wording?

This patch is meaningless without the parent, but merging this in there just adds noise.

amateescu’s picture

Well, if we're sure the parent issue will land, I suggest to simply take out that part of the comment(s) :)

lauriii’s picture

Status: Reviewed & tested by the community » Needs work

Now that the parent issue is critical I agree with @amateescu that we shouldn't add the documentation and can be pretty sure that it will land sooner or later.

  • alexpott committed df7ced5 on 8.0.x
    Issue #2561259 by stefan.r, lauriii, joelpittet, andypost: Cast (...
alexpott’s picture

Status: Needs work » Reviewed & tested by the community

Let's just remove the comments in #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list. Since atm the string casts are necessary but once that lands they are necessary. As for #36.1 won't #342316: Introduce proper Form API #types for 'option' and 'optgroup', and make #options consistent. fix this?

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed df7ced5 and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

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