Quoting @moshe weitzman from the Drush issue queue.

The safe_join function needs to be enhanced so that does not error when descriptions is empty. Obviously, twig-compile has no value for descriptions. This would be a Drupal core patch.

This currently makes Drush return the below errors:

$ drush @site.env twigc
Invalid argument supplied for foreach() twig.engine:170                                                                       [warning]
Invalid argument supplied for foreach() twig.engine:170

FYI, the twig-compile command compiles all Twig templates.

CommentFileSizeAuthor
#9 improve_the_safe_join-2540870-9.patch2.3 KBjoelpittet
#9 interdiff.txt1.38 KBjoelpittet
#9 improve_the_safe_join-2540870-9--tests-only.patch1.19 KBjoelpittet
#4 improve_the_safe_join-2540870-4.patch1.89 KBlauriii
#4 improve_the_safe_join-2540870-4-test-only.patch807 byteslauriii
#1 improve_the_safe_join-2540870-1.patch776 bytesjoelpittet

Comments

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Title: Improve the safe_join in file-upload-help.html.twig » Improve the |safe_join Twig to be inline with twig_join_filter()
Status: Active » Needs review
StatusFileSize
new improve_the_safe_join-2540870-1.patch776 bytes

This is a good point. Here's a patch. Likely will have to go into 8.1.x?

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Seems fair and also seems like we could test it.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Assigned: Unassigned » lauriii

Writing tests (adding kittens)

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Assigned: lauriii » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new improve_the_safe_join-2540870-4-test-only.patch807 bytes
new improve_the_safe_join-2540870-4.patch1.89 KB

The last submitted patch, 4: improve_the_safe_join-2540870-4-test-only.patch, failed testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Version: 8.0.x-dev » 8.1.x-dev
Category: Task » Bug report
Priority: Normal » Major

We may need to add this in a quick bug fix release in 8.0.1 or something... I'm not sure but this is broken as it is.

I'll bump it to 8.1.x if other people run into this then maybe we need to include it early.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 8.1.x-dev » 8.0.x-dev
Issue tags: +Contributed project blocker, +rc target triage

I think given that this creates errors in drush does this not mean this is a contrib project blocker? And something we might want fix during rc?

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -545,10 +545,14 @@ public function renderVar($arg) {
+    if ($value instanceof \Traversable) {
+      $value = iterator_to_array($value, false);
+    }

Is this tested?

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

@alexpott thanks I think it's tested by Twig but not in D8.

https://github.com/twigphp/Twig/blob/1.x/test/Twig/Tests/Fixtures/filter...

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
StatusFileSize
new improve_the_safe_join-2540870-9--tests-only.patch1.19 KB
new interdiff.txt1.38 KB
new improve_the_safe_join-2540870-9.patch2.3 KB

Here is a test for that too.

The last submitted patch, 9: improve_the_safe_join-2540870-9--tests-only.patch, failed testing.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Looks good, tests look to be comprehensive now.

The last submitted patch, 4: improve_the_safe_join-2540870-4-test-only.patch, failed testing.

The last submitted patch, 9: improve_the_safe_join-2540870-9--tests-only.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: -rc target triage +rc target

Discussed with @catch, @xjm, @effulgentsia and agreed this should be rc target.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed a53d5aa and pushed to 8.0.x. Thanks!

  • alexpott committed a53d5aa on 8.0.x 
    Issue #2540870 by joelpittet, lauriii: Improve the |safe_join Twig to be...

Status: Fixed » Closed (fixed)

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