Part of meta-issue #1788918: [GTD] [META] Prepare for Twig main engine core inclusion until December 1
template_preprocess() provides 4 arays to assist with theming content:
attributes_array
classes_array
title_attributes_array
content_attributes_array
I am not sure why classes_array exists, and why it isn't incorporated into attributes_array[]class]. Alternatively, I don't know why title_attributes_array and content_attributes_array don't have title_classes_array and content_classes_array pairs.
It seems that things would be easier to use, and more understandable, if there was consistency here.
Comment | File | Size | Author |
---|---|---|---|
#91 | drupal-1290694-91.patch | 87.25 KB | tstoeckler |
#91 | interdiff-87-91.txt | 1.13 KB | tstoeckler |
#88 | drupal-1290694-88.patch | 87.21 KB | tstoeckler |
#88 | interdiff-87-88.txt | 1.08 KB | tstoeckler |
#87 | drupal-1290694-87.patch | 87.23 KB | tim.plunkett |
Comments
Comment #1
andypostsubscribe
Comment #2
effulgentsia CreditAttribution: effulgentsia commentedAt last weekend's San Francisco theme system sprint, chx wrote an Attributes class that implements ArrayAccess and __toString() that can allow us to simplify to a single $attributes variable that can automatically be an array when it needs to be, a string when it needs to be, and contain $attributes['class'] that can similarly handle array/string magic, and let the template choose whether to print class (or any other attribute) separate from other attributes or whether to just print $attributes containing everything. All in all, awesome stuff.
hefox started on preparing a patch that updates core to do all of the above. @hefox: any eta on when you can post a work-in-progress patch here?
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedTagging and changing component to "theme system", since this doesn't affect the markup itself.
See also #1484704: Remove instances of attributes_array for a different, but related issue.
Comment #4
hefox CreditAttribution: hefox commentedHere's a patch with the attributes class.
The one problem I know about is that
doesn't work (cause it doesn't know that class needs to be a attribute array class, etc.) and produces an ugly error. Looked briefly and looks like there may be a way around it based on something said on php.net, but wasn't able to figure it out.
works fine:
or
I think I updated most places, but likely missed a spot. Been a week since patch was created.
Comment #5
tim.plunkettRerolled to still apply to core. Looks interesting for sure.
Comment #6
chx CreditAttribution: chx commentedComment #7
c4rl CreditAttribution: c4rl commentedIn template_preprocess_html():
Shouldn't we adhere to arrays here?
That way we can safely use things like in_array() in preprocessors.
Comment #8
c4rl CreditAttribution: c4rl commentedAlso in theme_form_element():
Seems like this should be
I'm working on these cleanups in #7 and #8.
Comment #9
c4rl CreditAttribution: c4rl commentedActually, nevermind #8, that's outside the scope of this issue. :)
Comment #10
c4rl CreditAttribution: c4rl commentedRe-rolled with fixes from #7.
Comment #12
tim.plunkett#10: drupal-1290694-10.patch queued for re-testing.
Comment #14
c4rl CreditAttribution: c4rl commentedOops, forgot to include Drupal\Core\Template\Attribute ... re-rolling shortly.
Comment #15
c4rl CreditAttribution: c4rl commentedThere was an problem with Attribute::offsetSet as the $name parameter could be null. Let's try this one.
Also made Drupal\Core\Template\Attribute compatible with PSR-0.
Comment #18
chx CreditAttribution: chx commentedComment #20
chx CreditAttribution: chx commentedOne by one, the free templates of Drupal lands fell to the power of the Attributes...
Comment #21
chx CreditAttribution: chx commentedComment #23
chx CreditAttribution: chx commentedI do not have more time tonight, whoever is going to work on this next please when you start working on it, use the Assigned field so we don't cross each other.
Comment #24
hefox CreditAttribution: hefox commentedComment #25
hefox CreditAttribution: hefox commentedLocal test is fataling, so seeing what happens here.
Comment #26
hefox CreditAttribution: hefox commentedComment #28
hefox CreditAttribution: hefox commentedAssuming I got the patch recreated correctly, this should fix the node title failure.
the node.tpl.php change needed to be
Edit: this should fix everything but the HTML Attributes test.
Comment #29
hefox CreditAttribution: hefox commentedCan't get CommonDrupalAttributesUnitTestCase to run locally, atm (fatals), so here's a stab.
Comment #30
hefox CreditAttribution: hefox commentedhm, the host of new failures is, uh, interesting.
Comment #31
hefox CreditAttribution: hefox commentedComment #32
RobLoachBig fan of the clean up in this patch! Also really like the use of ArrayAccess. These are PHP tools that we definitely should take more advantage of. Some minor notes.....
We should probably have "use Drupal\Core\Template\Attribute;" at the top and "return new Attribute()" instead.
Is there any reason for this change?
Absolutely love this clean up :-) .
In some of the PSR-0 patches, I believe we had \ArrayAccess and \Twig_Markup instead of the use statements. I'm okay with either though :-) . It's just minor syntax. We could re-evaluate those later on globally.
Would be good to have a docblock for Attribute and the other classes.
Comment #33
Crell CreditAttribution: Crell commentedUm, they shouldn't have. Current coding standards say to "use" everything, even internal classes, and never have inline \. (Vis, what this patch is doing is correct.)
Comment #34
hefox CreditAttribution: hefox commentedchx from irc: RobLoach: as for $elements = call_user_func($function, $elements); it does not belong here but http://cdn.memegenerator.net/instances/400x/21190468.jpg
Current action items for this based on above two comments:
I can update it tonight when I get home assuming I don't forget, but if I do forget or anyone sooner gets to it, go at it :).
Comment #35
hefox CreditAttribution: hefox commentedI'm not very good at writing comments :/
Comment #37
hefox CreditAttribution: hefox commentedForgot to get rid of the drupal_render
Comment #38
hefox CreditAttribution: hefox commentedComment #39
hefox CreditAttribution: hefox commentedComment #41
hefox CreditAttribution: hefox commentedComment #42
hefox CreditAttribution: hefox commentedComment #43
RobLoachLooks great! Patch applied cleanly and everything is rendered exactly how it was before. Cleaner code, and we have more control over how it's outputted.
Mind sticking notes in the issue summary to follow ups where we actually make use of Twig_Markup? Thanks!
Comment #44
catchWe're manually instantiating a new class here, three times for every call to template_process(). I'd like to know how expensive this is. Also it looks like we could potentially keep a static with the result of drupal_attributes(array('class' => array()); then copy that rather than building a new one from scratch?
This isn't really 'flatting out' is it any more?
array.syntax - space instead of a full stop?
Does this actually work with ArrayAccess? I didn't think it could handle offsets yet.
i.e. you can do $foo = $bar['baz']; $foo['a'][] = 'b'; $bar['baz'] = $foo; but I don't know if just $foo['bar']['a'][] = 'b'; will work.
Typo for 'individual'.
OK this answers my earlier question then, we're using ArrayAccess inside ArrayAccess to do this, nice :)
This isn't a sentence...
Base classes are supposed to be suffixed 'Base' now per coding standards.
Can we explicitly mention the Attributes class here? 'object of HTML attributes' is a bit non-committal.
In template_preprocess() we were unconditionally assigning the attributes class, here we're keeping the ternary, why?
I love lines like this :)
Comment #45
hefox CreditAttribution: hefox commentedThe problem with showing invisible characters, sometimes periods look like spaces :P.
We're manually instantiating a new class here, three times for every call to template_process(). I'd like to know how expensive this is.
I'd like to know this also, but I don't know how to adequately test that (this is one of my main concerns with this patch).
Base classes are supposed to be suffixed 'Base' now per coding standards.
I was looking for example, but couldn't find it in the object oriented coding standards (wasn't sure if it'd be AttributeValueBase or AttributeBase, etc.), only the simpletest ones. Is this a pending coding standard? Changed to it though
Added a static to template_proprocess, but makes the question, should drupal_attributes have a static $default that it clones if the given array is blank?
Also, should drupal_attributes check and see if it's being called again (e.g. it's already Attributes object) and then just return the object? Seems safer but not sure the standards on that.
Comment #46
hefox CreditAttribution: hefox commentedDid I just accidently do a combo of blockquote and code above instead of just blockquote.. oops.
Comment #48
hefox CreditAttribution: hefox commentedDidn't rename AttributeValue.php
Comment #50
hefox CreditAttribution: hefox commentedhmm
Comment #51
hefox CreditAttribution: hefox commentedComment #53
hefox CreditAttribution: hefox commentedReverted the use of clone.
Start with [class] in it already so as it's common enough to want class (as an array) in various places having to add the if (!isset(attributes class)) attributes class = array()) seemed undesirable.
That proves problematic for doing clone on a default drupal_attributes(array('class'=> array()) as the attributeArray 'class' object isn't cloned (since not a deep clone).
Could implement a copy constructor (assume php has one), not sure if it's warranted. Thoughts?
Comment #54
tim.plunkettI think a good deal of the field-related fixes between #48 and #54 are actually due to #1625158: template_preprocess() doesn't run for theme_field()
Comment #55
Crell CreditAttribution: Crell commentedI don't believe PHP has an explicit copy constructor. You'd use clone() instead, and the __clone() magic method to handle deep cloning.
Whether that's a good or bad performance trade-off to just creating a new object, I have no idea.
Comment #56
hefox CreditAttribution: hefox commentedShould just be an update to latest 8.x of 53
Comment #57
hefox CreditAttribution: hefox commentedThis is with clone
Using a devel/php:
with output
0.013292074203491 drupal_attributes 64976
0.0047738552093506 clone 1680
So assuming I didn't mess that up, clone is significantly better.
Comment #58
hefox CreditAttribution: hefox commentedbad patch
Comment #59
hefox CreditAttribution: hefox commentedAlternate to clone in template_preprocess could do something like:
Comment #60
quicksketchThis patch looks pretty cool, though half-way implementations of "things that act like arrays but really are objects" can be frustrating to work with. This patch currently only implements ArrayAccess, but not Iterator. This means that you can't actually loop through all the available attributes, you can only check if known attributes exist or not. If we're going to be implementing ArrayAccess, we should also implement Iterator.
This seems like an odd restriction. I would think that if you wanted to call
print $someAttributeObject
, then modify it, then print it again, it would reflect the changes.In the documentation for Attributes, I'd love to see a more explicit PHPdoc that said, "Most of the time this object is not created directly, but instantiated by drupal_attributes()." The small @see drupal_attributes() understates the actual common usage.
There are a couple references to "Twig_Markup" classes. These are probably a bit premature to include in this patch.
Comment #61
c4rl CreditAttribution: c4rl commenteddrupal_common_theme() was setting $variables['attributes'] to default to be an empty Array, and was bypassing class Attribute. I removed these, which also required checking isset($attributes) in Attribute::__construct to avoid PHP warnings.
Comment #62
c4rl CreditAttribution: c4rl commentedBad patch. Sorry, that is_set was a silly change that was related to something else I was doing. :P
Comment #64
msonnabaum CreditAttribution: msonnabaum commentedI was seeing a significant regression with the latest patch (~10%), but we found an unnecessary check_plain() which accounted for most.
Here are the before and after with the attached patch, using the APC autoloader.
Also attached an ab diff, which slows a slightly larger regression, but I don't think the wall time diff is terribly meaningful in this case.
Comment #65
mlncn CreditAttribution: mlncn commented*Everything* with this seems to work in my testing *except* the front page of Drupal when there are no nodes.
Somehow printing the title or calling drupal_set_message must be weird here-- the latter is called with PASS_THROUGH as a parameter?
It gives the error:
Warning: Invalid argument supplied for foreach() in Drupal\Core\Template\Attribute->__construct() (line 42 of core/lib/Drupal/Core/Template/Attribute.php).
And replaces the "Welcome to @site-name" with "Error"
Again, only on the front page without nodes is this a problem, everything else, and if any node is promoted, or a different front page set, it works.
Comment #66
chx CreditAttribution: chx commentedHrm, I think actually the one that msonnabaum uploaded is correct although it does revert the removal of array()s but I can not reproduce those on HEAD so I think it was a mistake during the other Twig work. Looking at theme_item_list, all the attributes are passed through drupal_attributes.
Comment #68
chx CreditAttribution: chx commentedOh that's because msonnabaum rerolled at the last min against c4rl's. This is what I meant.
Comment #69
tlattimore CreditAttribution: tlattimore commentedThis patch is a re-roll of chx's in #68 that removes the call to Twig_Markup, which quicksketch expressed concerns about in #60.
Comment #71
tlattimore CreditAttribution: tlattimore commentedI forgot got add the core/lib/Drupal/Core/Template directory before diffing for the patch. Here is a re-roll of #69 which includes that, and will hopefully pass testing.
Comment #73
tlattimore CreditAttribution: tlattimore commentedI mistakenly dropped all offsetSet() in Attribute.php do to a misunderstanding from chx in irc. This patch is simply a re-roll of #68.
Comment #74
tlattimore CreditAttribution: tlattimore commentedThis is exactly the same as #73, but also adds the documentation changes to Attribute.php that quicksketch requested in #60.
Comment #76
tlattimore CreditAttribution: tlattimore commentedMissed a closing bracket in Attribute.php. Somehow removed it in the midst of futzing with re-rolling the patch.
Comment #77
mlncn CreditAttribution: mlncn commentedThank you, tlattimore, and huge credit to hefox, tim.plunkett, c4l, msonnabaum, and chx. This is good to go in!
Comment #78
tstoecklerI really love (!!) this patch, but I'm afraid I have to mark it needs work once more.
Any reason to keep drupal_attributes() around, since it is just a trivial wrapper? (There probably is a reason, I'm just asking.)
These, and all of the other functions and variables in the Attibrute* classes should be documented. I don't know what the standard is for stuff like __toString(), but I suspect we have a standard :)
Seems like this could print the attributes directly, no?
Seems we could gain a feature at no cost, by simply printing attributes here?!
Comment #79
chx CreditAttribution: chx commented> Any reason to keep drupal_attributes() around, since it is just a trivial wrapper? (There probably is a reason, I'm just asking.)
Yes. That's a trivial followup. This is already a 80kb hard-to-read patch.
> These, and all of the other functions and variables in the Attibrute* classes should be documented
Hardly. Most method are just implementing ArrayAccess. In this patch I documented those that aren't.
> Seems like this could print the attributes directly, no?
Per jenlampton what template people most want is just to be able to add a class so while
<body class="<?php print $attributes['class']; ?>" <?php print $attributes;?>>
might look wasteful it's a TX boon because you can add a class without any PHP code.I added iterators to our arrayaccess objects -- thanks to IteratorAggregate that was mighty easy. Edit: Funny when you Google IteratorAggregate the first non-php.net result is fabpot's blog post.
Comment #80
chx CreditAttribution: chx commentedTagging.
Comment #81
mlncn CreditAttribution: mlncn commentedAnd good to go again. @tstoeckler thank you for the review. Function __toString is as close to self-documenting as we can get, as are the other short methods within the Attributes class. Every method being documented is not a standard that i'm aware of and should not hold up this patch.
Comment #82
tstoecklerNot marking "needs work" again, but from http://drupal.org/node/1354#functions:
And then in http://drupal.org/node/1354#classes:
Comment #83
tim.plunkettLooking at http://api.drupal.org/api/search/8/offsetGet for example, @tstoeckler is right. It can't hurt, I'll do this now while I unwind from work. :)
Comment #84
tim.plunkettI followed the lead of existing code in core, php.net docs, and http://drupal.org/node/1354#classes.
Re-RTBC, but there's an interdiff.
Comment #85
tstoecklerCool, awesome. RTBC++
Comment #86
neclimdulSadness to complain about docs again but we really should document the argument since this our own implementation no an overridden method. I'd roll it myself but I'm not at my development machine.
Comment #87
tim.plunkettRan this by neclimdul in IRC, so marking back to RTBC.
Comment #88
tstoecklerSorry for not seeing this earlier, but for __construct() we do:
(Where Foo is the class name.)
To not hold this up any longer attached patch with that change. Interdiff for easy review.
Leaving at RTBC, because it's such a small change, and - with the interdiff - should be verifiable by a committer. Hope that doesn't put anyone off.
Comment #89
tstoecklerActually, setting back to a quick "needs review". Again, should not really be hold-up.
Comment #90
tim.plunkettConstructs a Drupal\Component\Plugin\Factory\DefaultFactory object.
Constructs a Delete object.
Initializes a new ArrayCollection.
Constructs a new cache backend.
Implements Drupal\Core\Config\StorageInterface::__construct().
I fail to see a pattern. If we don't go with the patch in #87, I think we should use the full namespace for the class.
Comment #91
tstoecklerOh, maybe you're right.
Looking at our coding standards: http://drupal.org/node/1354#classes
The following is noted there:
That said the class there is not in any namespace, so...
I noted the need for a standard for class constructors in #1487760-45: [policy, no patch] Decide on documentation standards for namespaced items
Uploading a patch that uses the fully-qualified namespaces. Either this or #88 is RTBC, so marking as such. The comitter can decide which one is preferred. Since we already have an issue for defining and establishing a standard regarding namespaces (#1487760: [policy, no patch] Decide on documentation standards for namespaced items), working code should not be held up on that decision, IMO.
Comment #92
RobLoach#91 is RTBC. The documentation update is minor, but does remove the meaningless doc block referencing itself. Let's stop arguing about minor doc syntax and get this in so it unblocks all the other issues that depend on this.
Comment #93
Crell CreditAttribution: Crell commentedI know I'm late to the party here, but if drupal_attributes() is a one liner around new Attribute(), why bother with the function? It's just an extra stack call and a hard function dependency we don't need.
-24 days to next Drupal core point release.
Comment #94
RobLoachGood call... Made a follow up for that one: #1700382: Replace remaining references to drupal_attributes() with new Attributes().
Comment #95
chx CreditAttribution: chx commentedThis was raised in #78 and in #79 I did say it's a followup. Thanks for filing it tho so we wont forget.
Comment #96
Crell CreditAttribution: Crell commentedAh, cool. Sounds good to me.
Comment #97
effulgentsia CreditAttribution: effulgentsia commented#91: drupal-1290694-91.patch queued for re-testing.
Comment #99
tim.plunkett#91: drupal-1290694-91.patch queued for re-testing.
Comment #100
chx CreditAttribution: chx commentedThe tests passed after a bot fluke so back to RTBC.
Comment #101
Dries CreditAttribution: Dries commentedCommitted to 8.x. Nice clean-up that hopefully helps new designers and themers. Thanks!
Comment #102
aspilicious CreditAttribution: aspilicious commentedThis deserves a change notice. This affects several modules I know....
Comment #103
aspilicious CreditAttribution: aspilicious commenteddamnit!
Comment #104
sunTagging.
Comment #105
tim.plunkettJust for ... consistency!
Comment #106
hefox CreditAttribution: hefox commentedI have no idea what I;m doing, but here's an attempt at a change notice: http://drupal.org/node/1727592
Comment #107
tstoecklerI just removed the reference to drupal_attributes as that is on its way out. Otherwise looks good. I haven't quite grasped the nesting of Attributes yet myself, so cannot judge whether that is adequately documented. Hence, not setting RTBC.
Comment #108
tim.plunkettIt should mention drupal_attributes, but as the D7 way to do things. It is very helpful to have contrasting D7/D8 snippets to show how to update any D7 code using the old way.
Comment #109
chx CreditAttribution: chx commentedI added a few words about the D7 drupal_attributes.
Comment #110
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #112
jenlamptonadding tag
Comment #112.0
jenlamptonminor edit
Comment #112.1
podarokUpdated issue summary.
Comment #113
mitokens CreditAttribution: mitokens as a volunteer commented