Refactor Attribute class into one class

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

Re-rolled.

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.

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.

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.

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

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

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

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*

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.

17: 2239945-attributes-refactor-17.patch queued for re-testing.

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

17: 2239945-attributes-refactor-17.patch queued for re-testing.

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

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

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:

• It's a PITA to merge attributes because they are nested objects in preprocess.
• $variables['attributes'], $variables['title_attributes'], $variables['content_attributes'] are still arrays till the last second before the template. @see #2108771: Remove special cased title_attributes and content_attributes for Attribute creation
• You can't do anything useful with attributes values in templates besides printing them out of order or excluding them with |without().

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.

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