Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
We need a way to be able to add in classes from the template files to the core/contrib provided attribute object's class value.
Proposed resolution
@see patch.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#85 | 2285451-attribute-addClass-removeClass-85.patch | 9.2 KB | joelpittet |
#85 | interdiff.txt | 965 bytes | joelpittet |
#85 | 2285451-attribute-addClass-removeClass-85-tests-only-fail.patch | 9.03 KB | joelpittet |
#74 | 2285451-attribute-addClass-removeClass-74.patch | 8.94 KB | joelpittet |
#74 | interdiff.txt | 1.49 KB | joelpittet |
Comments
Comment #1
joelpittetSo here's my POC, it could use more tests in an actual twig template but I did do some tests and it works there just the same.
Here is my test data for the twig template if someone wants to roll that into a new test.
Any renderable array with an Attribute object passed (eg usually $variables['attributes'] array on every hook_theme will do that).
Comment #2
mortendk CreditAttribution: mortendk commentedLooks good im gonna test it with this #2254153: Move node classes out of preprocess and into templates
Comment #3
RainbowArrayThis looks good to me too.
Comment #4
mortendk CreditAttribution: mortendk commentedThis looks good heres a patch for the node.html.twig file as described in #2254153: Move node classes out of preprocess and into templates
it looks a bit strange at a first - but it make sense :)
Comment #5
joelpittetMay be a bit confusing that you posted on this issue;) I'm hiding your patch and mine here... but we should move this over to the node issue for discussion. And maybe get support for this over there @mortendk?
Anyways, here's my refactoring of #4
I added #2283301: Add Twig filters for drupal_html_class() and drupal_clean_id_identifier() so we don't need any classes in preprocess.
Comment #6
mortendk CreditAttribution: mortendk commentedyeah i was a bit confused to figure out where to put it ( and between to worldcup games *coughs*) but the most important think is that the test went through *woho* and it works (more woo) Im fine with moving it over to the node issue
oooh had no idea about
thats pretty slick - but we need some documentation for it
Alright so im gonna move the patch to #2254153: Move node classes out of preprocess and into templates
Comment #7
joelpittet@mortendk Where is a good place to document that? That trickery is part of twig. Any method with Object.getSomething can be printed in twig with {{ object.something }} and same goes for the Object.isSomething convention can be checked with {% if object.something %} twig looks for array keys, object public properties and methods, then it goes and looks for magic methods like __get() and __isset().
Since we have the node object being passed to the template, we've got all it's public methods at our fingertips.
Comment #8
mortendk CreditAttribution: mortendk commented@joel I think we should put it in the comment header for the template, then its at the fingertips
lets carry this on over at #2254153: Move node classes out of preprocess and into templates
Comment #9
joelpittetThis may be our downfall, but it's what we are asking for... got some feedback from @webchick and maybe we should have this ability but not do this in core and leave this in preprocess?
Comment #10
dawehnerJust a drive-by review.
We could/should add @covers here for the phpunit code coverage report.
Are you sure there is no dedicated assert for that?
Comment #11
mortendk CreditAttribution: mortendk commentedno were not leaving anything in the preprocess and we should do this in core, its about time we break out of the drupal chains (chants with a fist in the air)
Comment #12
markhalliwellPersonally I would much rather see:
vs.
Comment #13
joelpittet@Mark Carver please add that to #2254153: Move node classes out of preprocess and into templates issue too. As it will serve more people over there.
Though because class is not a method but an object property I wonder if your example will work... we'll be sure to add that to a twig template test. It's that or we may need
{% attributes.addClass(...) %}
Comment #14
markhalliwell@joelpittet, I'm not sure what you're referring to. I am using the exact same thing as in the patch
attributes.class.add(classes)
, except instead of setting up a separate variableclasses
, I am just passing the array directly to the function. Thus, eliminating needless clutter.Comment #15
RainbowArray@joelpittet: The syntax markcarver used is functionally the same as what's in your patch.
You have:
So using an add method with an array parameter on attributes.class. That's exactly what markcarver is doing with:
That too is an add method with an array parameter on attributes.class.
This just lets us keep the element a lot cleaner, with a simple {{ attributes }}, by moving the add method up to where the classes are being set.
Comment #16
joelpittet@Mark Carver re #14 It will likely work, I just need to test because I'm not confident the references to the parent will work, so I'll just be sure to include that (sanity check) in the test.
Comment #17
jwilson3This seems like a duplicate of an idea that was discussed first 2 years ago first here: #1808254: Standardize and simplify the attribute syntax in Twig template files, and then formalized and spun off to a separate issue here: #1835396: Attribute modifier functions for themers. Interesting back story, and maybe there are some gems there, for the people that are taking this on now.
Comment #18
joelpittet@jwilson3 thanks for this, I followed and commented on that 2 years ago... didn't recall that issue. I don't think using filters are a good idea for attributes, but methods will work great because they are related to that object. Filters are usually working on simple types like string or array. But I do need to re-read that issue to see if I can't get some more ideas. attributes.addClass() I think is the way to go here and I'll rewrite this to that syntax and iterate from there.
Comment #19
cilefen CreditAttribution: cilefen commentedBecause this patch is needed to proceed now on #2289511: [meta] Results of Drupalcon Austin's Consensus Banana, we need this in. I tweaked code standards in this patch.
Comment #20
joelpittet@cilefen thanks sorry for the delay on this one. I'll tackle this in the evening tonight. Changing the title to reflect the direction this will be taking.
Same code just moved to the main Attribute class to make it look like jQuery as I've wanted to do and seems has historic precedence as @jwilson3 pointed out.
Comment #21
cilefen CreditAttribution: cilefen commented@joelpittet I'll try that now.
Comment #22
cilefen CreditAttribution: cilefen commentedHow can we add those methods to Attribute when it has already been cast to AttributeArray?
Comment #23
cilefen CreditAttribution: cilefen commentedForget that, I'm dumb...
Comment #24
cilefen CreditAttribution: cilefen commentedComment #25
joelpittetHa, well that's one way to do it, nice work @cilefen! I never thought of doing that... I was going to scrap my work on the add()/remove() methods and just move their guts to the new methods on Attributes, which we still may have to do, but for now that looks like it's green!
Nice, probably needs a more tests still.
Comment #26
joelpittetBah time is not on my side at the moment. #24 looks good, maybe we can get a few reviews before I change things further?
Comment #27
davidhernandezYeah so cilefen and I were hashing this out at CapitalCamp before adding the patch. Abstracting the functions made sense and was very clean. This also made sense having the add/remove methods themselves in AttributeArray as we are forcing 'class' to always be AttributeArray, though we want addClass and removeClass to be part of Attribute. I think this would maintain consistency if extrapolated in the future to add other methods
I'm uploading another patch, just fixing some comments. I'm also going to manually test it in some templates to make sure we are getting the results we need.
Comment #29
davidhernandezSorry, missed one. That last test didn't fail; we had it cancelled.
Comment #30
davidhernandezComment #31
joelpittet@davidhernandez that is looking very nice so far, thanks for the standards cleanup and addClass/removeClass additions. Likely we will talk about this at 4PST in the #drupal-twig hangout you are welcome to drop in if you are around.
Comment #32
joelpittetThis is now
attributes.addClass
andattributes.removeClass
I've added those covers @dawehner mentioned #10's drive by but couldn't find a PHPUnit assert for in_array().
You're welcome to try it out with your banana themes.
Here is my inside twig test, still haven't got a twig tests for the attributes setup.
Comment #34
joelpittetBoo that works in 5.5, oh well I should know better:P Likely don't need that check too, it's a micro optimization anyway.
Comment #35
cilefen CreditAttribution: cilefen commentedI know you are not finished the detailed twig test yet, but just watch out for these in the final:
commented code
commented code
var_dump
Comment #36
RainbowArrayI can't speak much to the way the code is written, but I like the way this works in the template.
We'll want to make sure in the change notice we have one or two good examples of how people would use this in a template. attributes.addClass(classes).class.value() feels clunky to me when we're chaining it like that. Would it be something like this?
Comment #37
davidhernandez@mdrummond, that is correct.
Comment #38
joelpittetYou don't need
.value()
That is because I wanted the array and I wanted to put new line characters instead of spaces for me to read it easier in my tests:)@cilefen thanks for spotting those leftover debug statements. Here's a cleaner version.
@mdrummond Yeah real world examples will be like yours in #36
Comment #39
joelpittetMinor nit pick with #36 don't put the space with
<div[NO SPACE HERE]{{ attributes }}>
BUT<div class="class[SPACE HERE NEEDED]{{ attributes.class }}"{{ attributes.without('class') }}>
Attributes put a space before every attribute printed and if there are non your tag doesn't have a weird empty space. Classes on the otherhand don't add a space in their value. I think it makes sense if you think about it?
Comment #40
davidhernandezFor adding twig test for this, what was envisioned? I full webtestbase to check the markup with classes?
Comment #41
davidhernandez@joelpittet, is that exchangeArray() function necessary? I don't want to nitpick though, so I don't care if it is there. Just curious why add it there, if you felt having the add() and remove() functions in AttributeArray was confusing. Wouldn't that also be confusing?
I manually tested #38, with modifications to templates. The expected output was rendered while removing and adding classes.
I'm not seeing an easy way of adding an actual twig test at this point. Because the templates have not been modified yet to take advantage of this new method, I believe a test module implementing a fake theme function would be needed to use a test twig file. That seems like overkill for this, when it can be tested for later after the templates have been modified. Shouldn't the unit tests be enough for now?
Comment #42
joelpittetFor the twig test, just similar items as the PHP test but in a twig file, and if we can avoid a full bootstrap of a webtest I'll try to do that too.
exchange array is necessary because I don't have access to the value() direclty but there is a myriad of other ways I could have handled that I tried something that was in PHP already so it's familiar in use.
As long as you can access the Attribute object and the Twig service it should just work. I'll take a crack at it here in a bit.
Comment #43
joelpittetThe unit test should be good to know that it works as expected for PHP, the twig one is just to confirm it works as expected in the twig template.
Comment #44
davidhernandezI agree the twig test is useful, I just couldn't figure out how to get it to work without too much effort. If you can do it, I'll review it as soon as I can. I just want to make sure we don't get stalled on that.
Comment #45
joelpittetI think it is good enough to test it out with the banana stuff so you are welcome to run with that patch it's in a good spot at the moment.
Comment #46
davidhernandezIt would be good to get this in before digging into the preprocess functions, if we can. Otherwise, we'll have to link them all here which will create some confusion/complication.
Comment #47
joelpittetTwig tests added to the same UnitTest so it should be much faster than a web test. Bunch of mountains.
Edit: Sorry wrong patch #.
Comment #48
joelpittetWell there was garbage in that one anyway. Removed here.
Comment #49
dawehnerJust a quick review ...
In case you wonder how to document it. There is a proposal how to do that in PSR5: https://github.com/phpDocumentor/fig-standards/issues/40 and an actual implementation in phpDocumentor https://github.com/phpDocumentor/phpDocumentor2/issues/629 This uses the php 5.5 syntax for variadic arguments
We do use @return $this to make it clear what exactly is returned.
two spaces ...
You could also use assertContains instead
Comment #50
joelpittet@dawehner thank you very much I never know how to document those 'variadics' nor did I know they had a name.
Hopefully I did it correctly here? Also I used
assertNotContains()
becauseassertContains()
would give some false positives but I did change the others to be more consistent toassertArrayEquals()
.Comment #51
dawehnerCool!
Comment #52
chx CreditAttribution: chx commentedI am sorry but
this is beyond not helpful: it doesn't make any sense and although English is not my first language, is "Classes arguments" even grammatically correct?? "Adds the arguments to the existing CSS classes". "CSS classes to add". Perhaps. Feel free to add better but there's not a word in the current doxygen indicating that addClasses is specific and hardwired to the 'class' key. Similarly for removeClass.
Comment #53
RainbowArrayPatch to address chx's concerns with the docblocks:
Comment #54
dawehnerI agree this is an improvement.
Comment #55
chx CreditAttribution: chx commentedThanks for fixing this!
Comment #56
alexpottIs this the correct documentation and shouldn't they always be strings?
Should we be worrying about uniqueness here too?
Comment #57
alexpottI think this might be fixed now... #2304399: "class" attribute in drupal_render should be an array
Comment #58
davidhernandezThe methods can be used with a string class name or array of class names.
Comment #59
alexpottSo then maybe it should be
Comment #60
davidhernandezIt can be used with a single argument:
<div{{ attributes.addClass('test') }}>
Multiple arguments:
<div{{ attributes.addClass('test1', 'test2') }}>
Or an array:
Are they all covered by string|array ?
Comment #61
alexpottThe unknown arguments (a variadic function) is what the
...
in the PHPDoc is about. But those arguments are expected to be either strings or arrays so we should tell people.Would work too :)
Comment #62
davidhernandezOh ok. So the 'type' part is strict to exactly what is expected?
Comment #63
joelpittetI was under the impression more than one type was documented as mixed... but I like that you can use the pipe to denote the options. Thanks for the tip @alexpott! Also added unique as mentioned in #54 which is a good idea, was originally just trying to keep it light as possible but I think that is still reasonable.
#57 maybe fixed but I'd like to enforce the rule instead of hoping people do it right. Even in core there was/is some accidental attribute.class strings.
Comment #64
joelpittetComment #65
joelpittetDrafted a change notice here https://www.drupal.org/node/2315471
Comment #66
davidhernandezI tested this manually in twig templates and still get the desired results.
Comment #67
alexpottAlso I'm confused why are we using array_filter? Just in case someone passes in
array('')
or an empty string? That seems too defensive?Also we should add a test for the use of array_unique.
We should not need to run array_unique here. See above comment about array_filter too.
Comment #68
davidhernandezAre you saying there is a better way to check for duplicates, or not to bother checking?
Comment #69
alexpottI'm asking why we are using array_filter in both the
addClass
andremoveClass
functions?Also the array_unique is definitely not necessary when calling
removeClass
.For some reason the dreditor snippets are not very helpful because i was not commenting on the offsetExists function.
Comment #70
joelpittet@alexpott you're right it is defensive. Maybe it would be better if moved the array_unique and array_filter calls to the AttributeArray when it's printed? Save a few CPU cycles?
Comment #71
joelpittetHow's this? I think this should be defensive but not sure where, so I'll save that for another issue because it may affect performance. Merging will deal with most duplicates (that aren't there already).
Comment #72
joelpittetWrong interdiff, hiding.
Comment #73
alexpottAdding empty values is poor code - adding duplicates well that is hard to defend against. So I'd prefer to drop the array_filter and just use array_unique. Also putting it here means we're now doing this for all AttributeArrays not just the class. Can we just add it to addClasses and be done.
Comment #74
joelpittet... OK, I'd like to clean empty values out as well, but I can do that in a separate issue, I guess.
Comment #77
penyaskitoRelated critical: #2218271: Theme preprocess functions have to futz with $page_object = $variables['page']['#page'] in weird ways
Comment #78
davidhernandezI tested this manually in templates and get the expected results in all the different ways. Duplicates are removed. Empty space is not, as expected. Technically, extra spaces don't break anything, and they can only get there from a module or template, so it shouldn't be a deal breaker.
Shall we RTBC?
Also, any more suggestions for the change record? https://www.drupal.org/node/2315471
Comment #79
RainbowArrayLooks to me like all of the issues have been addressed. I did another pass through, and this all seems pretty sensible to me.
Comment #80
joelpittetThanks @mdrummond;)
Comment #81
star-szrI would recommend the title of the change record be in the past tense.
Comment #82
davidhernandezPast tense is indeed in keeping with the other change records. I updated it.
Comment #83
star-szrGave it a further tweak so that it makes a better "headline" :) thanks @davidhernandez!
Comment #84
alexpottCan we add a test for the array_unique functionality.
Comment #85
joelpittetSure can!
Comment #87
davidhernandezNice, Joel. So the first patch should fail because it has the uniqueness test, but not the array_unique() filter. The second has the filter and the test, which passed.
Comment #88
alexpottCommitted 65cb382 and pushed to 8.0.x. Thanks!
Comment #90
davidhernandezAwesome. I published the change record.