Working on a patch and summary to help improve and simplify the token integration in Page Titles for D7 by re-adding the dependency on token module...

CommentFileSizeAuthor
#10 interdiff.patch801 bytespillarsdotnet
#8 page_title.patch14.94 KBpillarsdotnet
#7 page_title.patch14.09 KBpillarsdotnet
#4 942978-pagetitle-token-cleanup-followup-D7.patch3.98 KBdave reid
#1 942978-pagetitle-token-cleanup-D7.patch16.77 KBdave reid

Comments

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Active » Needs review
StatusFileSize
new 942978-pagetitle-token-cleanup-D7.patch16.77 KB

Patch summary:

  1. Re-adds the module dependency on token module in contrib. This is done to help re-use what's available in token module like...
  2. Token context validation on the page title fields. So with just a little code you get automatic errors if someone tries to use the [user:name] token on the Comment Reply pattern.
  3. Use of the 'token tree' UI so you don't have to do any custom token listing code.
  4. Moves the token implementation to page_title.tokens.inc. Note this is automatically supported by core and doesn't rely on Token module to just work.
  5. Move the page-title token underneath the [current-page] token namespace, which is provided by Token module and not by core. It also provides the [current-page:page-number] token so that's done for you.
  6. Use the proper $options['sanitize'] rather than providing both the page-title and page-title-raw tokens.

So sorry that it's not quite that easy to drop the dependency on Token module for D7, but we do want to help provide everyone with as much re-usable code that we can hopefully get into core for D8.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Issue tags: +token

Adding tag...

nicholasthompson’s picture

nicholasthompson
Primary language English
commented
Status: Needs review » Fixed

Commited to DRUPAL-7--2. Thank you so much for the patch. MUCH appreciated!

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Fixed » Needs review
StatusFileSize
new 942978-pagetitle-token-cleanup-followup-D7.patch3.98 KB

Some small follow-ups that somehow didn't make it into the original patch:
1. Support using vocabulary tokens on the forum container page title
2. Token replacement should request the unsanitized tokens since it applies filter_xss() later.

nicholasthompson’s picture

nicholasthompson
Primary language English
commented

Is there a coding preference amongst the community for comma's at the end of in-line arrays? http://drupal.org/coding-standards#array doesn't seem to state either way (apart from the example not having one)
Eg

$some_array = array('hello', 'world', 'foo' => 'bar');

vs

$some_array = array('hello', 'world', 'foo' => 'bar',);

Sorry if it seems picky - but I just noticed a few lines of that patch which only seem to change the trailing comma.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

If your array is on one line and less than 80 characters long, it's the code standard to not use a trailing comma. But if you have a long array with each element on a separate line, you're supposed to use a trailing comma. It's detailed exactly on http://drupal.org/coding-standards#array

pillarsdotnet’s picture

pillarsdotnet commented
StatusFileSize
new page_title.patch14.09 KB

Re-rolled with some additional fixes:

  • Wrap long comments at 80 columns.
  • Minor grammatical corrections within comments.
  • Wrap long conditional expressions at infix operators.
  • Add parentheses around assignment statements within conditional expressions.
  • Collapse nested conditional statements where possible.
  • Improve logic in function node_page_title_alter().
pillarsdotnet’s picture

pillarsdotnet commented
Title: Re-add token module dependency and cleanup token integration » Cleanup token integration
StatusFileSize
new page_title.patch14.94 KB

More of the same.

nicholasthompson’s picture

nicholasthompson
Primary language English
commented

Ah I just applied the patch from #7 and refreshed here to reply... What changed between #7 and #8?

pillarsdotnet’s picture

pillarsdotnet commented
StatusFileSize
new interdiff.patch801 bytes

Interdiff is your friend.

nicholasthompson’s picture

nicholasthompson
Primary language English
commented

Oooo Interdiff! Handy!

nicholasthompson’s picture

nicholasthompson
Primary language English
commented
Status: Needs review » Fixed

Committed - thanks for the work - much appreciated!

Status: Fixed » Closed (fixed)

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

porg’s picture

porg commented

Dear maintainer: Could you please add a short info concerning Drupal 7 and module dependency to the project page so that not every ordinary user must snoop through the issues queue, in order to get that basic info. Thanks! Something like:

Although the functionality of the contributed module token has moved into core in Drupal 7, page_title is still dependent on the module token to support some functions yet not included in core. (For details read: #942978: Cleanup token integration ).

pillarsdotnet’s picture

pillarsdotnet commented

@porg

You either need to re-open this issue (change its status to "needs work") or preferably, open a new issue and then add a comment here that links to it.

porg’s picture

porg commented
Status: Closed (fixed) » Needs work