Problem/Motivation

The file core/lib/Drupal/Core/Entity/Plugin/Derivative/DefaultSelectionDeriver.php defines the class Drupal\Core\Entity\Plugin\Derivative\DefaultSelectionDeriver, but the documentation block for the constructor mentions a different class name:

  /**
   * Creates an SelectionBase object.
   *
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   *   The entity type manager.
   */
  public function __construct(EntityTypeManagerInterface $entity_type_manager) {

This mismatch is probably left over from the changes described in this change record.

This documentation block generates the API documentation for DefaultSelectionDeriver::__construct(), so I am adding the "API Documentation" issue tag.

Proposed resolution

I am not sure whether "SelectionBase" should be replaced by "DefaultSelectionDeriver" (the name of the deriver class) or "DefaultSelection" (the name of the base class). The conventional code comment for A::__construct() is "Creates a(n) A object", but maybe the convention is different for deriver classes. Look for other examples in Drupal core.

There is another code comment, later in the same file, that refers to "SelectionBase", and that one should be replaced with "DefaultSelection".

Remaining tasks

User interface changes

None, except for fixing the API documentation.

API changes

None

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

benjifisher created an issue. See original summary.

reinchek’s picture

Hi i've modified the comments into class' file. Before i've looked for other Deriver classes' comments and it's always written

Creates a(n) ClassName object.

Thanks,
Nino.

benjifisher’s picture

Status: Active » Needs work

@reinchek:

Thanks for taking this on. Looking at your profile, I see that this could be your first core issue. I hope it is not the last!

When you upload a patch, you should also set the issue status to Needs Review (NR). That will trigger the testbot. I will do that for you.

... it's always written

Creates a(n) ClassName object.

But is ClassName the name of the deriver class or of the base class being derived? Can you add a link to the API documentation for one of the examples you found?

On second thought, I will set the status to Needs Work (NW):

-   * Creates an SelectionBase object.
+   * Creates an DefaultSelectionDeriver object.

It should be "a" rather than "an".

reinchek’s picture

Hi @benjifisher! thanks for the info! yes it's my first contribution to the core... in this sector I am currently a novice!

Ok for the NR status!

But is ClassName the name of the deriver class or of the base class being derived?

Yes ClassName is the name of the deriver class not the base class one. Follow you can see some examples (in constructors' docblock):

Sometimes the phrase is "Constructs a(n) ClassName" instead of "Constructs a ClassName object" but i think that this isn't a standardized detail.

It should be "a" rather than "an".

I attached the new patch with "a" modification.

Thanks,
Nino.

reinchek’s picture

Status: Needs work » Needs review
benjifisher’s picture

InlineBlockDeriver is a funny case, since BlockContentDeriver is not the name of any class as far as I can tell. There is a BlockContentDeriverTest class. I have a feeling that there was too much copy/paste and not enough update when InlineBlockDeriver was added.

I am not sure that ViewsEntityArgumentValidator is a relevant example. Although it is in a Plugin\Derivative namespace, it does not have Deriver in its name. Drupal core seems to be inconsistent in whether deriver classes have Deriver in their names.

I think that \Drupal\Core\Entity\Plugin\DataType\Deriver\EntityDeriver<.a> and \Drupal\Core\Field\Plugin\DataType\Deriver\FieldItemDeriver are better examples. These examples support your decision.

Just for practice, please attach an interdiff. I do not really need it for this issue, but it helps for reviewing more complex patches. See Novice contributing 3: Create a patch and/or Creating an interdiff.

benjifisher’s picture

Status: Needs review » Needs work

I am setting the status to NW for the interdiff.

reinchek’s picture

@benjifisher
yes you right; InlineBlockDeriver is a funny case! I forgot to mention the third example that used the correct name.
in any case I used different class type (like ViewsEntityArgumentValidator) as example because i don't think that there's a standard for docblock to use for the constructor that differs from the one of other class type.

After I will attach the interdiff.

ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
1.03 KB

Here is interdiff of patch #2 and #4

benjifisher’s picture

Status: Needs review » Needs work

@ravi.shankar:

How did you produce that interdiff? I get something similar from diff -u 3098921-1.patch 3098921-4.patch (a diff of the two patch files) but that is not a proper interdiff. Please have another look at the documentation I mentioned in #6. (If you were following the instructions there, then we need to correct the documentation!)

I am setting the status back to NW because I still want an interdiff.

shubham.prakash’s picture

Status: Needs work » Needs review
FileSize
878 bytes

Added the interdiff as asked in #6

benjifisher’s picture

Status: Needs review » Reviewed & tested by the community

@shubham.prakash:

As I said in #6, the point of asking for an interdiff was so that a novice can get some practice. I see from your profile that you have already contributed to many issues, including several core issues.

The patch in #4 looks good, and the interdiff in #11 compares it to the first attempt, so I will mark this issue RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 011ea4a70b to 9.0.x and 275d8b7860 to 8.9.x. Thanks!

I credited @benjifisher for mentoring
I didn't credit @shubham.prakash and @ravi.shankar as @benjifisher was asking for this for practice and not for the issue per se.

  • alexpott committed 011ea4a on 9.0.x
    Issue #3098921 by reinchek, benjifisher: DefaultSelectionDeriver has...

  • alexpott committed 275d8b7 on 8.9.x
    Issue #3098921 by reinchek, benjifisher: DefaultSelectionDeriver has...

Status: Fixed » Closed (fixed)

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