Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Drupal\views\EventSubscriber\RouteSubscriber::routeRebuildFinished() is undocumented.
The method's docblock currently consists of a {@inheritdoc} tag, but this method is not declared by any ancestor class.
The method is a callback for the RoutingEvents::ALTER event, declared by the getSubscribedEvents() method in the same class.
Proposed resolution
Replace the {@inheritdoc} tag with an explanation of what the method does.
Remaining tasks
- Decide what documentation should say.
- Follow API documentation and comment standards.
User interface changes
None.
API changes
None.
Data model changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | document-routeRebuildFinished-2847655-12.patch | 675 bytes | diaspar |
Comments
Comment #2
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThis will probably be a one-chunk patch, good for a new contributor.
Comment #3
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedComment #4
ruloweb CreditAttribution: ruloweb at Media.Monks commentedAssigned to me for on of the new contributors in the SprintWeekend2017 Buenos Aires
Comment #5
xjmComment #7
diaspar CreditAttribution: diaspar as a volunteer commentedThis my first contribution to Drupal Core. Please review and let me know if it's ok.
Comment #8
diaspar CreditAttribution: diaspar as a volunteer commentedneeds review
Comment #9
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedYou need to Change status to "needs review" after uploading patches for testing them.
Comment #10
ruloweb CreditAttribution: ruloweb at Media.Monks commentedComment #11
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThanks for the patch in #7, @diaspar! Here's a review.
Overall, the new docblock follows the right general structure. Looking at the individual lines:
1.
This is correct, but it mostly just repeats the code inside the method body. It could be better if it summarized the overall purpose of the method.
Elsewhere in the class,
$this->viewRouteNamesis described as "an array of route names" rather than objects. The important part of this method is the call to$this->state->set()which stores these newly-generated route names using the State API. The call to$this->reset()is just a bit of clean-up.Something like "Stores the new route names after they have been rebuilt."
2.
This can be shortened to "Callback for the RoutingEvents::FINISHED event", per the documentation standards for callback implementations.
3.
This is a good use of
@see, pointing to the where the callback was was registered. However the separator between class name and method name should be a double-colon, like so:+ * @see \Drupal\views\EventSubscriber::getSubscribedEvents()A good first patch, @diaspar. If you can make a new patch to address the feedback, I can review it again. Remember to set the issue status back to "Needs Review".
Comment #12
diaspar CreditAttribution: diaspar as a volunteer commentedThanks for your clear help @andrewmacpherson.
I am attaching a new Patch
Comment #13
diaspar CreditAttribution: diaspar as a volunteer commentedComment #14
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThanks @diaspar.
The patch in #12 addresses all the feedback in comment #11.
RTBC - let's pass this on to the committers. Nice first patch!
Comment #17
catchCommitted/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!