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

CommentFileSizeAuthor
#20 drupal-rename_mark_type_to_status-2010672-20.patch3.14 KBjenlampton
#16 2010672-16.patch3.19 KBstar-szr
#12 2010672-12.patch3.85 KBstar-szr
#12 interdiff.txt461 bytesstar-szr
#10 2010672-10.patch4.3 KBstar-szr
#10 interdiff.txt3.84 KBstar-szr
#2 2010672-2.patch5.76 KBthedavidmeister
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Assigned: Unassigned » thedavidmeister
thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Assigned: thedavidmeister » Unassigned
Status: Active » Needs review
FileSize
2010672-2.patch5.76 KB
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

This 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()?

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented

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

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

I thought the issue here was that this doesn't work:

$mark = array(
  '#theme' => 'mark',
  '#type' => node_mark($node->nid, $node->changed),
);

As far as I can see the patch here does not remove the theme hook, so would still allow us to do:

$mark = array(
  '#theme' => 'mark',
  '#mark_type' => node_mark($node->nid, $node->changed),
);

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

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented

Yeah, 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));

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

If 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 :)

thedavidmeister’s picture

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 :)

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Assigned: Unassigned » star-szr
Status: Needs review » Needs work

Thanks - will look at this tonight.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Title: Convert #theme mark to #type mark » Rename 'type' variable of theme_mark to 'mark_type'
Assigned: star-szr » Unassigned
Status: Needs work » Needs review
FileSize
interdiff.txt3.84 KB
2010672-10.patch4.3 KB

Here's the patch, re-titling.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Status: Needs review » Needs work 
+  $types['mark'] = array(
+    '#theme' => 'mark',
+  );

This would now be irrelevant too I think, so pull it out :(

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs work » Needs review
FileSize
interdiff.txt461 bytes
2010672-12.patch3.85 KB

Ah good point.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented

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

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs review » Reviewed & tested by the community

Then let me. :-)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work

Needs a reroll

curl https://drupal.org/files/2010672-12.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  3943  100  3943    0     0   1476      0  0:00:02  0:00:02 --:--:--  1580
error: patch failed: core/modules/node/node.admin.inc:542
error: core/modules/node/node.admin.inc: patch does not apply
error: patch failed: core/modules/node/node.module:1916
error: core/modules/node/node.module: patch does not apply
error: patch failed: core/modules/tracker/tracker.pages.inc:77
error: core/modules/tracker/tracker.pages.inc: patch does not apply
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs work » Needs review
FileSize
2010672-16.patch3.19 KB

Rerolled.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs review » Reviewed & tested by the community

Again.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Title: Rename 'type' variable of theme_mark to 'mark_type' » Change notice: Rename 'type' variable of theme_mark to 'mark_type'
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed 0744c0d and pushed to 8.x. Thanks!

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Active » Needs review

Change 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:

  • Module developers
  • Themers

Sound good? Anything missing?

jenlampton’s picture

jenlampton
she / her
CreditAttribution: jenlampton commented
Title: Change notice: Rename 'type' variable of theme_mark to 'mark_type' » Rename 'type' variable of theme_mark to 'status'
FileSize
drupal-rename_mark_type_to_status-2010672-20.patch3.14 KB

We 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 for status 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:

  • Module developers
  • Themers
jenlampton’s picture

jenlampton
she / her
CreditAttribution: jenlampton commented
Issue tags: +Novice, +Twig

tagging for a quick review :)

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs review » Reviewed & tested by the community

I like #status.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Active

Committed ef3335c and pushed to 8.x. Thanks!

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Novice, -Needs change record

Created a small change notice:

https://drupal.org/node/2030821

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Updated issue summary.