Follow-up and postponed on #2227869: Remove magic printing from Attribute class to make Twig 'without' filter work for attributes.

Remove the architecture around printed attributes of instantiating a new object for each attribute's value.

Files: 
CommentFileSizeAuthor
#29 2239945-attributes-refactor-29.patch25.75 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,504 pass(es). View
#29 interdiff.txt917 bytesjoelpittet
#27 interdiff.txt2.45 KBjoelpittet
#27 2239945-attributes-refactor-27.patch25 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,620 pass(es), 7 fail(s), and 254 exception(s). View
#17 2239945-attributes-refactor-17.patch25 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#17 interdiff.txt10.38 KBjoelpittet
#13 2239945-attributes-refactor-point-proof-not-real-solution.patch22.49 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,326 pass(es), 6 fail(s), and 461 exception(s). View
#9 2239945-attributes-refactor-9.patch18.13 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,819 pass(es), 22 fail(s), and 4,623 exception(s). View
#3 2239945-attributes-refactor-3.patch17.37 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#1 2239945-attributes-refactor-1.patch28.67 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2239945-attributes-refactor-1.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

joelpittet’s picture

Status: Postponed » Needs review
FileSize
28.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2239945-attributes-refactor-1.patch. Unable to apply patch. See the log in the details link for more information. View

This has a few more little things so posting to sort those out with the help of testbot's help.

Status: Needs review » Needs work

The last submitted patch, 1: 2239945-attributes-refactor-1.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
17.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 3: 2239945-attributes-refactor-3.patch, failed testing.

Rajendar Reddy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: 2239945-attributes-refactor-3.patch, failed testing.

joelpittet’s picture

Thanks @Rajendar Reddy, though I found a critical issue in my approach, now that the values of the object array are values, the class(and a few other rdf attributes) are array values and when we split them and {{ print }} them separately they are being treated as numerically indexed arrays and passed through drupal's render() at the end of twig_render_var().

So that fails outright with exceptions from checks Element::children().

It would be awesome to have the same values as entered, and manipulation of those values would be much less complicated in preprocess and in twig, but they need the magic __toString() rendering that they are wrapped in. So may have to take a different approach here. Any suggestions or experiments welcome:) But I'll likely post something within the week.

sun’s picture

Issue tags: -Needs change record

+1

All of those Attribute classes + rendering always appear very high in my profiling. Consolidating them should/might allow us to additionally improve the rendering performance.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
18.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,819 pass(es), 22 fail(s), and 4,623 exception(s). View

I think I've found a neat little trick that makes this work the way I was hoping it would!

__call() get's used by twig Twig_Template::getAttribute. So I can use that for property getting in twig, but in PHP the values get treated as they were entered! Not sure this is perfect, but it seems pretty damn close so lets see if testbot agrees.

Status: Needs review » Needs work

The last submitted patch, 9: 2239945-attributes-refactor-9.patch, failed testing.

joelpittet’s picture

This can work still if I can force Twig_Template's getAttribute to pass along a Twig_Template::METHOD_CALL for Attribute.

This would be the best scenario because the array is left untouched until it or it's values need to be rendered by twig.

joelpittet’s picture

FYI, if you change {{ attributes.class }} to {{ attributes.class() }} #9 will work...

I think we can use a NodeVisitor to check A) It's being printed, B) it's a Twig_Node_Expression_GetAttr and C) Are we dealing with an Attribute object... (C) is the tricky part...

joelpittet’s picture

Status: Needs work » Needs review
FileSize
22.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,326 pass(es), 6 fail(s), and 461 exception(s). View

Just to prove to people and myself #12 would work, here's a patch.

Status: Needs review » Needs work
joelpittet’s picture

Well... I probably missed a couple places but you can see by the fail/exception differences...

joelpittet’s picture

Assigned: Unassigned » joelpittet
Issue tags: +Needs issue summary update

Assigning and I've got a less magic plan to do this. Will need some good issue summary update and a good Change record for this but I think it will be for the best, *crosses fingers*

joelpittet’s picture

Status: Needs work » Needs review
FileSize
10.38 KB
25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Ok so the plan:

Remove the magic that is printing attribute values.

Leave the values entered as the values entered! Don't convert them to objects that know how to print themselves.

The Attribute object still knows how to print itself with it's magic __toString() which is still awesome sauce in my books.

The PRO-tractors

  1. The values however as soon as they are created were cast to objects making them a huge PITA to merge in preprocess because merging objects as if they were arrays is not easy to think about or do.
  2. Putting one thing in and asking for it back and getting another thing is a DX WTF.
  3. From a TX/FX/TwigX POV merging in a class just didn't work before eg. class="{{ attributes.class|merge(['clearfix']) }}" because twig_merge() checks to see is_array() and Object/ArrayAccess/ArrayObject != array And if you can't merge in the template you get possible duplicate classes when 'clearfix' is already in the attributes.class array and you get an empty space before if attributes.class is empty.
  4. Merging now possible! {{ attributes.class|merge(['field--bem-class', 'field--inline']).join(' ') }} @see http://twig.sensiolabs.org/doc/filters/merge.html
  5. Same empty space problem goes for splitting out boolean attributes which you can see in action for the twig_theme_test.filter.html.twig which I fixed here too with the syntax ternary sugar {{ attributes.checked ? ' checked' }}

The decepti-CONS:

  1. It's more verbose... You have to add |join(' ') because it is an array.
  2. Since it's an array if you print it, drupal_render() will be called on it like it's a renderable array and you get a nasty ass
    "0" is an invalid render array key

Also fixed a few totally wrong attribute setting up pieces in this patch too that were made evident by the change in how they are rendered.

dawehner’s picture

+++ b/core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.filter.html.twig
@@ -13,10 +13,10 @@
+<div><span class="{{ attributes.class|join(' ') }}"{{ attributes|without('class') }}>Class attributes in front, remainder at the back:</span></div>

Can't you just make a "class" method on the attributes class? This seems to be a special case we can cover.

Status: Needs review » Needs work

The last submitted patch, 17: 2239945-attributes-refactor-17.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
joelpittet’s picture

@dawehner I'm not totally understanding what you are asking in #18.

I did make attribute.class() a method in #13 for shits and giggles but it was kinda weird that it was both a value and a method. We could do that too if people think that's in anyway an improvement because it would be a shorter version of #17 with all the same features...

Then both of these would work.

<span class="{{ attributes.class|join(' ') }}"{{ attributes|without('class') }}>
<span class="{{ attributes.class() }}"{{ attributes|without('class') }}>

Just need a bit more documentation because if you forget the parenthesis or |join(' ') you still get that error mentioned in #17.

dawehner’s picture

Oh mh ... i thought twig can figure it out automatically that in case you have a method on a class which is public it is called here.

So foo.class would call a method internally, but you as a themer won't have to care about it.

joelpittet’s picture

@dawehner twig can do that, but in this case the Object also is ArrayAccess and twig picks that up first in the internals(my guess is because it's faster than checking if the method call exists and calling it).

I wanted that magic you are asking for, but I can't think of a viable solution to make it do that. So #17 is my solution to the problems we are having with Attribute.

Status: Needs review » Needs work

The last submitted patch, 17: 2239945-attributes-refactor-17.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 17: 2239945-attributes-refactor-17.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,620 pass(es), 7 fail(s), and 254 exception(s). View
2.45 KB

Looks like some tests got added so they needed some un-complicating.

Status: Needs review » Needs work

The last submitted patch, 27: 2239945-attributes-refactor-27.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
917 bytes
25.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,504 pass(es). View

One more little change to fieldset.html.twig Crossing fingers...

Jeff Burnz’s picture

After this patch how are we to add new classes or other attributes to new Attribute() objects, .e.g. say I add one in preprocess.

mdrummond’s picture

I don't understand why we're putting this merge/join attribute stuff in the template. It's not at all intuitive at first glance what's happening with that. Why not handle this merging in preprocess?

What are the use cases for having these merges handled in the templates? How is that useful to themers?

markcarver’s picture

In IRC yesterday, we discussed this and I have now slept on this. I agree, having merge/join in templates just to add simple classes seems counter intuitive. I get that Twig has this functionality, but it's intended use is to manipulate data for rare or very complex templates. It is not intended to be used in everyday use and quite frankly would annoy the crap out of me if I had to type this each time. This feels like a step backwards in FX.

Yes, we all know the ins and outs of Twig's advanced functionalities, but the average themer will look at this and go "WTF".

This issue needs a summary update explaining what is trying to be accomplished here.

markcarver’s picture

Status: Needs review » Needs work
joelpittet’s picture

I've thought about this solution I've proposed and it won't work. The removal of the name and value escaping makes this useless unless autoescape is turned on by default or we enforce the method call {{ attributes.class() }}. So I'm taking this solution off the table.

I do however feel very strongly that we are not thinking about the bigger picture here so here's some things to consider to hopefully clarify:

We are working with data:
You should be able to still access and manipulate the original data in a controller, preprocess, template anywhere it's used or passed into. Which currently we lose the original data because it's converted in the offsetSet() call of the ArrayAccess interface. So why even build attributes as an array with (string, boolean, array values) in the first place if we just cast them to objects on the way in?

My guess is that that comes from D7 land and arrays are fast and easy manipulate. But we've packaged them up and now they are not fast or easy to change. It seems we decided that the string, boolean, array values was a good way to think of these values in the simplest terms to build up in preprocess but when we move to the template those are too hard. Is it too complicated for themers to understand that attributes.class is an array and attributes.checked is a boolean?

Also the {{ attributes.class|join(' ') }} is for illustrative purposes of the possibility, I well expect core and most themers to just print the whole thing: {{ attributes }} and not worry their heads about it's guts until they *need* to change it then they will go to the docs.

The problems still exist:

I'm going to have to move the fixes I did in this issue to another issue. I'm sure I've got a clean up attributes issue laying around which isn't getting attention... #2187113: Incorrect usage of attributes in twig templates resulting in possible duplicate attributes.

joelpittet’s picture

Status: Needs work » Closed (duplicate)

#2285451: Create addClass() and removeClass() methods on Attribute object for merging css class names. Fixes the class merging issue which is the biggest of the merging PITAs.

Closing this as a duplicate. Going to try an embrace Attribute as an Object and not as an an Array that it pretends(poorly) to be.

joelpittet’s picture

Assigned: joelpittet » Unassigned