We have added a new convention for .twig files which was not in phptemplate files which I believe is a bit too verbose. Our templates have two @see references in the Doxygen such as:

* @see template_preprocess()
 * @see template_preprocess_aggregator_feed_source()

The first line above, @see template_preprocess(), is the one that should be removed IMO. It is excessive helpfulness.

To change this, we should remove the recommendation from https://drupal.org/node/1823416 so we are back to the Coding Standards doc at https://drupal.org/node/1354#templates which does not require that line. In any case, this discrepency needs fixing

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Component: documentation » theme system
Status: Active » Reviewed & tested by the community

I totally agree with this proposal. I'm not sure why the @see template_preprocess() was put in there. The second (specific) @see I think is more helpful.

This seems like total common sense, and no one ever looks at coding standards issues until they are RTBC, so I am tentatively setting to RTBC. We should not take action until some time has passed and/or a few more core contributors have weighed in.

If we could get some theme system people to weigh in, that would be good, so I'm temporarily setting the component to theme system too.

jhodgdon’s picture

Title: Stop saying '@see template_preprocess()' in every twig file » [policy, then patch] Stop saying '@see template_preprocess()' in every twig file

Forgot to change title to the standard coding standards issue title format.

star-szr’s picture

star-szr’s picture

I should add: It looks like the code sample in the Twig coding standards was taken from region.tpl.php, so I don't think the @see template_preprocess() was added there intentionally.

jhodgdon’s picture

That's very interesting. All of those tpl.php files listed had the @see template_preprocess() in them, even though it is not part of the current coding standards for tpl.php files:
https://drupal.org/node/1354#templates
and it is, it seems we all agree, overkill to put it on every tpl.php file.

Anyway. We should leave it for a few more days just in case someone else wants to comment, then update the standards if there is no disagreement.

catch’s picture

Agreed here as well.

jhodgdon’s picture

Title: [policy, then patch] Stop saying '@see template_preprocess()' in every twig file » [policy adopted, patch needed] Stop saying '@see template_preprocess()' in every twig file
Status: Reviewed & tested by the community » Active

OK, I went ahead and updated the standards page to remove the see for template_preprocess(), since I have seen no disagreement.

The next thing we need is a patch to remove all of those lines. Should we do one big patch? It seems like it would be pretty easy to generate this patch with a script, pretty easy to review it, and it would be unlikely to conflict with other patches, right?

jenlampton’s picture

Issue tags: +Novice

Awesome. I'm also 100% behind this.
and more tags.

pwieck’s picture

Assigned: Unassigned » pwieck

working on this

pwieck’s picture

Assigned: pwieck » Unassigned
Status: Active » Needs review
FileSize
28.89 KB

Removes all @see template_preprocess() in .twig file only.

Status: Needs review » Needs work
Issue tags: -Novice, -Coding standards, -Twig

The last submitted patch, theme-system-remove-see-template_preprocess-2013094-10.patch, failed testing.

pwieck’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, theme-system-remove-see-template_preprocess-2013094-10.patch, failed testing.

pwieck’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Coding standards, +Twig
star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @pwieck!

I think one patch is fine for this - any conflicts will be very easy to resolve and I think there will be very few. Pending patches that are adding Twig templates with this line should probably be updated at this point to remove the @see line. Sub-issues of #1757550: [Meta] Convert core theme functions to Twig templates and issues like #1337554: Develop and use separate branding for the installer come to mind.

I reviewed the patch, it changes everything it should and nothing it shouldn't. Looks great to me.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Indeed, thanks for the patch and the review!

There were no conflicting "avoid commit conflicts" tagged issues, so I just committed this patch to Drupal 8.x. 57 .twig files were updated.

jhodgdon’s picture

star-szr’s picture

Thanks very much @jhodgdon :)

jwilson3’s picture

I updated the Drupal Twig conversion instructions google doc (maybe no one is using this anymore, but oh well, for completeness sake).

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