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
Comment | File | Size | Author |
---|---|---|---|
#11 | interdiff_2-4.txt | 878 bytes | shubham.prakash |
#4 | 3098921-4.patch | 1.36 KB | reinchek |
#2 | 3098921-1.patch | 1.37 KB | reinchek |
Comments
Comment #2
reinchekHi i've modified the comments into class' file. Before i've looked for other Deriver classes' comments and it's always written
Thanks,
Nino.
Comment #3
benjifisher@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.
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):
It should be "a" rather than "an".
Comment #4
reinchekHi @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!
Yes ClassName is the name of the deriver class not the base class one. Follow you can see some examples (in constructors' docblock):
* Constructs an ViewsEntityArgumentValidator object.
* Constructs a BlockContentDeriver object.
Sometimes the phrase is "Constructs a(n) ClassName" instead of "Constructs a ClassName object" but i think that this isn't a standardized detail.
I attached the new patch with "a" modification.
Thanks,
Nino.
Comment #5
reinchekComment #6
benjifisherInlineBlockDeriver 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.
Comment #7
benjifisherI am setting the status to NW for the interdiff.
Comment #8
reinchek@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.
Comment #9
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere is interdiff of patch #2 and #4
Comment #10
benjifisher@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.
Comment #11
shubham.prakash CreditAttribution: shubham.prakash at OpenSense Labs commentedAdded the interdiff as asked in #6
Comment #12
benjifisher@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.
Comment #13
alexpottCommitted 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.