Currently theme_mark() takes one variable, which is #type.
This is a problem because of #2005970: In renderable arrays, #type should provide a #theme suggestion, similar to how drupal_prepare_form() works and #2006152: [meta] Don't call theme() directly anywhere outside drupal_render().
We need to rename the existing variable to not be #type so that we can convert all array('#theme' => 'mark') type renderable arrays to array('#type' => 'mark')
Might I suggest #mark_type for simplicity?
Issues this is blocking:
- #2009578: Replace theme() with drupal_render() in history module
- #2005970: In renderable arrays, #type should provide a #theme suggestion, similar to how drupal_prepare_form() works
Comment | File | Size | Author |
---|---|---|---|
#20 | drupal-rename_mark_type_to_status-2010672-20.patch | 3.14 KB | jenlampton |
#16 | 2010672-16.patch | 3.19 KB | star-szr |
#12 | 2010672-12.patch | 3.85 KB | star-szr |
#12 | interdiff.txt | 461 bytes | star-szr |
#10 | 2010672-10.patch | 4.3 KB | star-szr |
Comments
Comment #1
thedavidmeister CreditAttribution: thedavidmeister commentedComment #2
thedavidmeister CreditAttribution: thedavidmeister commentedComment #3
star-szrThis makes sense and would nicely remove a render API WTF :) I wonder if we should be removing the theme() calls in this patch though - feels a bit out of scope to me. Could we leave those to the sub-issues of #2006152: [meta] Don't call theme() directly anywhere outside drupal_render()?
Comment #4
thedavidmeister CreditAttribution: thedavidmeister commentedThose sub-issues are technically converting theme('foo') to array('#theme' => 'foo') which is the key we're removing here so it doesn't really fit 100% there either.
I figured we could avoid some double handling if I just did the conversion straight up and patches touching these theme() functions over there were just rerolled.
Comment #5
star-szrI thought the issue here was that this doesn't work:
As far as I can see the patch here does not remove the theme hook, so would still allow us to do:
…at least as an intermediary step on the way to #2005970: In renderable arrays, #type should provide a #theme suggestion, similar to how drupal_prepare_form() works. Correct me if I'm wrong :)
I was basically looking for this issue to be more along the lines of #1828536: Rename 'type' variable of theme_item_list() to 'list_type', happy to roll a patch doing that if you agree.
Comment #6
thedavidmeister CreditAttribution: thedavidmeister commentedYeah, you could still do #theme => 'mark' with the patch I rolled. But if that was committed I would just immediately open a new issue "convert #theme => 'mark' to #type => 'mark'" and literally the only things I'd be converting would be the new renderable arrays introduced by this patch.
There are no existing renderable arrays for mark, because they aren't currently possible to construct without throwing errors.
The difference between this issue and the item list issue is that there are already renderable array versions of item lists in core so there's a reason to convert in two steps - you want to prepare the compatibility first and then convert all the remaining arrays which is easier to do in two steps.
If you want to keep the scope of this issue to a minimum keep theme(), don't introduce #theme just to have it changed a week from now. I'd suggest doing this instead:
theme('mark', array('mark_type' => node_mark($node->nid, $node->changed));
Comment #7
star-szrIf I'm understanding correctly we will need another batch of conversions for #2005970: In renderable arrays, #type should provide a #theme suggestion, similar to how drupal_prepare_form() works, won't we?
Your code sample at the end of #6 was what I was planning for the patch, sorry I should have been clearer about that :)
Comment #8
thedavidmeister CreditAttribution: thedavidmeister commented#7 - yes. We will need to do everything again for that linked issue, but it won't be as smooth as the first round because we have to find and fix all the bugs along the way.
I'm hoping that when we finish the second conversion the patch I put up already on that issue comes back green without any further changes.
You're welcome to reroll this one :)
Comment #9
star-szrThanks - will look at this tonight.
Comment #10
star-szrHere's the patch, re-titling.
Comment #11
thedavidmeister CreditAttribution: thedavidmeister commentedThis would now be irrelevant too I think, so pull it out :(
Comment #12
star-szrAh good point.
Comment #13
thedavidmeister CreditAttribution: thedavidmeister commentedThis patch looks RTBC to me if the testbots come back green but I don't know if I can set the issue to that status that considering that I rolled the first one.
Comment #14
tstoecklerThen let me. :-)
Comment #15
alexpottNeeds a reroll
Comment #16
star-szrRerolled.
Comment #17
tstoecklerAgain.
Comment #18
alexpottCommitted 0744c0d and pushed to 8.x. Thanks!
Comment #19
star-szrChange record draft:
theme_mark() 'type' variable is now 'mark_type'
To avoid ambiguity with the '#type' key in render arrays and pave the way for further changes and improvements, the 'type' variable in theme_mark() has been renamed to 'mark_type'.
Affects:
Sound good? Anything missing?
Comment #20
jenlamptonWe can do better than
mark_type
. Let's not use an underscore in a variable name when it's not necessary, and when we can we should be using a variable name that actually means something to our theme devs. I vote forstatus
since a 'new' or 'updated' flag indicates the status of the thing it's marking.New patch attached. new change record:
theme_mark() 'type' variable is now 'status'
To avoid ambiguity with the '#type' key in render arrays and pave the way for further changes and improvements, the 'type' variable in theme_mark() has been renamed to 'status'.
Affects:
Comment #21
jenlamptontagging for a quick review :)
Comment #22
tstoecklerI like #status.
Comment #23
alexpottCommitted ef3335c and pushed to 8.x. Thanks!
Comment #24
star-szrCreated a small change notice:
https://drupal.org/node/2030821
Comment #25.0
(not verified) CreditAttribution: commentedUpdated issue summary.