The documentation formatting for the existing twig templates in pixelmord's sandbox is fairly inconsistent. People looking at existing templates as example of how to document other templates are going to perpetuate these errors.

Not all of the implementers have followed the format recommended in the Drupal Twig conversion instructions.

I've gone through and tried to bring some consistency. Patch in first comment. Alternatively, commit access to the repo would be good.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jwilson3’s picture

Status: Active » Needs review
FileSize
90.62 KB

Things I've cleaned up:

  • Convert all doc sections to docblock format.
  • Added @file to top of docblock.
  • Rewrite first-line description on a single line less than 80 characters. (Still runs over in a couple cases).
  • Moved most @TODOs to a consistent place just above the @see section in the docblock. Inline @TODOs were left where they are in the html.
  • Added @see template_preprocess to all templates.
  • Removed parens after @see template_preprocess().
  • Added @ingroup themable to all templates.
  • Created a section "Available variables:" in all templates.
  • Removed '$' from some $variables.
  • Removed empty line between end of docblock comment and start of HTML output.
  • Ensure file ends with a newline.
  • Remove trailing whitespace.
Fabianx’s picture

Fantastic work!

Commit access granted! :-) Thanks a lot!

soulston’s picture

Great work @jwilson3

There has been some discussion about if we should output the variables as they would be output in the templates to make for more clarity and easier copy/paste http://drupal.org/node/1804710.

Should we use this:

//**
 * @file
 * Default theme implementation to display a node.
 *
 * Available variables:
 * - {{ label }}: the (sanitized) title of the node.
 * - {{ content }}: An array of node items. Use {{ content }} to print them all,

as opposed to this:

/**
 * @file
 * Default theme implementation to display a node.
 *
 * Available variables:
 * - label: the (sanitized) title of the node.
 * - content: An array of node items. Use {{ content }} to print them all,

It would be good to make the decision one way or the other. I prefer the use of the {{ item }} as it makes it easer to discern what the variables are, easier to copy paste and prevents ambiguity in the case where we might document twig tags eg. {% if page != true %}.

Fabianx’s picture

jwilson3’s picture

Status: Needs review » Closed (fixed)

Thanks, patch committed.

I've subscribed to #1804710 and would be happy to update existing docs again assuming the {{ notation }} is adopted.

Anyone feel free to reopen this if other twig files need formatting help, as they roll in.

Fabianx’s picture

We are continuing in #1759168: Comments in twig files [policy, no patch], but it seems {{ }} is out. So docs should be fine.

jenlampton’s picture

Just a follow up here...

We talked about this in the weekly twig meeting today, and we decided that the existing syntax should be sufficient.

In PHPTemplate, we did NOT write this in the docs:

/**
 * @file
 * Default theme implementation to display a node.
 *
 * Available variables:
 * - <?php print $title ?>: the (sanitized) title of the node.
 ...

so there is no reason to include the print statements for Twig, either.

There will be plenty of examples how the variable is being printed, and evaluated, in the Twig code below, and we expect most front-end devs to copy and paste from there as well.

jwilson3’s picture

Assigned: Unassigned » jwilson3
Status: Closed (fixed) » Active

In #1759168-15: Comments in twig files [policy, no patch], jhodgdon brings up a good point: "themeable" is misspelled in many twig templates. Reopening to fix that issue.

jwilson3’s picture

Done: http://drupalcode.org/sandbox/pixelmord/1750250.git/commit/926abb9

Note that in this commit, I've also fixed two instances outside of the stark/templates folder that were not additions from the work going on in this project and already exist in core. I'm not sure if this is a faux-pas or not. The misspelling in core/includes/form.inc appears to have been introduced in this branch, but the ones in install.core.inc and toolbar.module are preexisting issues in core.

jwilson3’s picture

Status: Active » Needs review
podarok’s picture

Status: Needs review » Reviewed & tested by the community

#9 looks good

jwilson3’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, marking fixed, since the commit is in.

jhodgdon’s picture

jwilson: Thanks for fixing those themeable misspellings -- I personally don't care if it's considered a faux pas or not, it's appreciated. :)

Status: Fixed » Closed (fixed)

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

star-szr’s picture

I've been reversing the following change made in #1 in the core queue:

Removed parens after @see template_preprocess().

Per http://drupal.org/node/1354#see the parens need to be there for Doxygen formatting since they are functions.

I'm going to ask about getting sandbox access so I can fix these.

star-szr’s picture

Thanks for all your work on this @jwilson3, I just made a couple commits here in the sandbox which should help during the transition to the core queue.

jwilson3’s picture

Status: Closed (fixed) » Active

Good point Cottser, thanks for pointing me to the API documentation and for fixing that up!

jwilson3’s picture

Status: Active » Closed (fixed)

oops