After working on #1927584: Add support for the Twig {% trans %} tag extension, I noticed that there is a LOT of documentation and code cleanup needed on Twig related stuff.

We should also probably be utilizing https://drupal.org/coding-standards/docs#defgroup properly to assert Twig in the "Themable" group and probably perhaps even a separate group just for Twig (ie: functions, filters, tags, etc.)

Files: 

Comments

jhodgdon’s picture

Are you talking about Twig templates or the functions Twig uses?

Twig templates already have a documentation standard:
https://drupal.org/node/1823416
which includes a defgroup.

markcarver’s picture

No I'm talking about the Twig classes/methods that have been introduced, like in #0. Ref: API documentation and comment standards

jhodgdon’s picture

I'm still not sure what you're advocating here.

Twig templates are supposed to be documented according to the Twig standards cited in comment #1, and this already includes @ingroup themeable for all Twig templates. If that standard is not being followed, then the templates that are not following the standard should be patched.

You should not use @ingroup themeable for anything but theme functions and templates, and given that I think we're trying to get rid of theme functions (right?), only Twig templates should have the "themeable" group in them.

If you want to make a new topic about functions related to processing Twig templates, or about things you can put into Twig templates, then feel free to introduce a new @defgroup. You'll need to come up with:
- A title for the topic
- A one-line short description of the topic
- One or more paragraphs of documentation about the topic for developers of contributed/custom modules and themes
- Which functions, classes, methods, etc. should be included in the topic

But I would only define a topic/group if there is some information to be imparted in the topic that is useful for developers to know. If it's just "this is the collection of all the functions that our Twig system uses", then that is probably not really worth doing a topic on.

markcarver’s picture

Ok, forgot I mentioned @ingroup themeable. I'm really not talking about templates.

I'm mainly talking about the classes that were introduced like: http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/lib/Drupal/Core/Template/TwigExtension.php

This is just one file, but you'll notice a lot of stuff pulled from Twig core/example extensions don't have proper comment blocks or adhere to Drupal coding standards. An example from the above file would be:

  public function getName()
  {
    return 'drupal_core';
  }

Should be:

  /**
   * {@inheritdoc}
   */
  public function getName() {
    return 'drupal_core';
  }

Re:

But I would only define a topic/group if there is some information to be imparted in the topic that is useful for developers to know. If it's just "this is the collection of all the functions that our Twig system uses", then that is probably not really worth doing a topic on.

I think there should be topic/groups like: twig_extension or twig_filter. This would allow developers to model new extensions off of core, which can be a little hard to find if you're not entirely sure where to look. This can be a separate issues though, not really what I'm trying to accomplish with this issue (just mentioning it for discussion sake).

jhodgdon’s picture

Sounds good! If there is information to impart (topic documentation), by all means make a topic/group. :)

Docs cleanup is always a good idea. If Twig stuff was committed without even minimal documentation on all classes, methods, and functions, then it's a violation of the Core Gates, and it shouldn't be like that.

markcarver’s picture

Agreed, we're doing more Core Gates on Twig issues now for sure.... but the initial Twig commit just added a lot of the Twig files directly without going through these gates. @jenlampton just wanted me to ping you to make sure I was doing this appropriately I guess.

Rene Bakx’s picture

Well i am not sure if this belongs here, but as you guys were talking about cleaning up. I fixed a small deprecated warning in the twig.engine file. So I thought might as well turn in into a patch.

Rene Bakx’s picture

Nooby patcher here, should have added my comment in the upload

Cottser’s picture

Status: Active » Postponed (maintainer needs more info)
Issue tags: +Needs issue summary update

This needs some direction, I know much of this has been covered by other issues along the way. Postponing for now until we can get something actionable.

joelpittet’s picture

Status: Postponed (maintainer needs more info) » Closed (duplicate)

Not sure which issue but this TwigExtension doc cleanup mentioned in #4 was done somewhere else.