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
Comment | File | Size | Author |
---|---|---|---|
#10 | theme-system-remove-see-template_preprocess-2013094-10.patch | 28.89 KB | pwieck |
Comments
Comment #1
jhodgdonI 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.
Comment #2
jhodgdonForgot to change title to the standard coding standards issue title format.
Comment #3
star-szrI am also fully on board with this change (theme system person :)). The Twig conversion effort did not add this convention, we brought this over from existing templates.
Handful of examples:
http://drupalcode.org/project/drupal.git/blob/4eaa775:/core/modules/syst...
http://drupalcode.org/project/drupal.git/blob/4eaa775:/core/modules/syst...
http://drupalcode.org/project/drupal.git/blob/4eaa775:/core/modules/taxo...
http://drupalcode.org/project/drupal.git/blob/4eaa775:/core/modules/syst...
Comment #4
star-szrI 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.
Comment #5
jhodgdonThat'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.
Comment #6
catchAgreed here as well.
Comment #7
jhodgdonOK, 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?
Comment #8
jenlamptonAwesome. I'm also 100% behind this.
and more tags.
Comment #9
pwieck CreditAttribution: pwieck commentedworking on this
Comment #10
pwieck CreditAttribution: pwieck commentedRemoves all @see template_preprocess() in .twig file only.
Comment #12
pwieck CreditAttribution: pwieck commented#10: theme-system-remove-see-template_preprocess-2013094-10.patch queued for re-testing.
Comment #14
pwieck CreditAttribution: pwieck commented#10: theme-system-remove-see-template_preprocess-2013094-10.patch queued for re-testing.
Comment #15
star-szrThanks @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.
Comment #16
jhodgdonIndeed, 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.
Comment #17
jhodgdonI also added a note to the meta issue #1757550: [Meta] Convert core theme functions to Twig templates
Comment #18
star-szrThanks very much @jhodgdon :)
Comment #19
jwilson3I updated the Drupal Twig conversion instructions google doc (maybe no one is using this anymore, but oh well, for completeness sake).