Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

Title: lib/Drupal/config_translation/ConfigMapperInterface.php standards cleanup to get ready for getting into core » ConfigMapperInterface.php standards cleanup to get ready for getting into core
Status: Active » Needs review
FileSize
1.7 KB

1.
  /**
   * Returns whether this configuration mapper needs an edit tab.
   *
   * @return bool
   *   TRUE if this mapper needs an edit tab, FALSE otherwise.
   */
  public function hasEditTab();

This is called *has* edit tab. But looking at the mappers that implement this interface, it's if it needs and edit tab.
Will needing a edit tab be the same as having an edit tab? Could something need an edit tab, but not have one?
This is probably ok.


2.
  /**
   * Returns the menu item type to be used for this mapper.
   *
   * @return int
   *   The menu item type used for this mapper.
   */
  public function getMenuType();

Is the menu item type different than the menu type?
What are some examples of menu types or menu item types?
I'm guessing menu types.. are those build in to the default profile, each menu is it's own type.
Then there are also custom menu's those have the type: custom menu type.
I think maybe contrib might be able to add other menu types.
For menu link items, what are their types? Are they the menu's they are in, the bundle? Or is it something more generic?


3.
  /**
   * Returns the path ID index if applicable.
   *
   * @return int|null
   *   The path ID index for the object ID in the path, or NULL if not
   *   applicable.
   */
  public function getPathIdIndex()

I'm not sure where to look this up, but when a word is all caps, like ID, how should it be in the method name? Id or ID? I think we ran into that when adding something for a standard time format or some other issue. I did a grep for URL vs Url in function names and there seem to be both...
AH! here it is.

If an acronym is used in a class or method name, make it CamelCase too (SampleXmlClass, not SampleXMLClass). [Note: this standard was adopted in March 2013, reversing the previous standard.]

from https://drupal.org/node/608152#naming
So Id is just right.


4.
  /**
   * Returns entity type of the entity type mapper, NULL otherwise.
   *
   * @return mixed
   *   The entity type of the mapper, or NULL.
   */
  public function getType();

when should getType return NULL?


5.
  /**
   * Returns route name for the group.
   *
   * @return string
   *   Route name for the group.
   */
  public function getRouteName();

This used to say just returns group name. Is the route name usually the same as the group name?

YesCT’s picture

The blocking stuff in there was,
a doc block on the class (interface), types on the params, and only @return when returning something.

Other questions are to make sure naming is making sense,
and there were also some typos fixed (which is just nits and not part of the doc core gate, so not blocking).

YesCT’s picture

YesCT’s picture

1. was really needsEditTab. so changed that.

2. was really getMenuItemType. so changed that.

3. was ok.

4. updated docs to clarify.

5. was ok.

Gábor Hojtsy’s picture

Status: Needs review » Fixed

We reviewed this together with YesCT in Dublin just now. It all looks good, committed!

Status: Fixed » Closed (fixed)

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