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.
/**
* Provides a breadcrumb manager.
*
* Holds an array of path processor objects and uses them to sequentially process
* a path, in order of processor priority.
*/
class BreadcrumbManager implements BreadcrumbBuilderInterface {
Appears to be copy/paste from PathProcessorManager.
Comments
Comment #1
dawehnerComment #2
jhodgdonYeah, yet another example of no one bothering to review documentation for the Documentation Gate. Sigh.
Thanks for reporting this!
As it was a violation of the Documentation Gate that this completely wrong documentation was included in a patch that was committed: in accordance with what catch did on some other issues recently that are also violations of the Documentation Gate, I am upping the priority to critical.
Comment #3
kgoel CreditAttribution: kgoel commentedComment #4
msonnabaum CreditAttribution: msonnabaum commentedWe might want something a bit more descriptive than that.
This class initially flagged my attention because it's called BreadcrumbManager, implements BreadcrumbBuilderInterface, and has a method to add BreadcrumbBuilderInterface. This suggests to me that it's a BreadcrumbBuilderBuilder, which is a bit odd.
Some documentation that explains why a BreadcrumbBuilderBuilder exists would be helpful I think.
Comment #5
klausiWhy is this critical? Yes it is bad that we missed the docs gate with a commit, but this does not render the system unusable or introduces a security bug. Let's stick to our issue priorities: https://drupal.org/node/45111
Comment #6
jhodgdonClasses with completely wrong documentation are not usable by other developers, so one could argue that it does "render the system unusable".
Anyway, I was just following catch's example of other "completely violated the documentation gate and should never have been committed" issues being marked as "critical". But I'm not going to start a flame war by changing it back. Whatever. We just need to fix it.
Comment #7
ghazlewood CreditAttribution: ghazlewood commentedHere's a stab at a better description of the class, hopefully I've not got the wrong end of the stick.
Comment #8
klausiTrailing white space, missing punctuation. And I think the first line should be a short description under 80 characters as its own paragraph.
Comment #9
paulh CreditAttribution: paulh commentedOk, BreadcrumbManager class description cleaned up as per suggestion. Also re-assigned to me, as I wan't sure if ghazlewood was coming back for this one.
Comment #10
paulh CreditAttribution: paulh commentedUpon reflection, the description could have been a bit clearer. Updated.
Comment #11
paulh CreditAttribution: paulh commentedCorrected for Drupal API documentation standards:
Comment #12
taslett CreditAttribution: taslett commented@paulh I've reviewed patch 11, it makes sense and meets the documentation standards.
Since it is a minor issue have changed the status to Reviewed & tested by community.
Comment #13
paulh CreditAttribution: paulh commentedComment #14
jhodgdonThanks for the patch and review!
To me... This doesn't actually make any sense. If I just read the class description (without reading the entire class file, other methods, etc.) it doesn't stand alone.
Also, the grammar is not right -- the subject of the sentence added is plural, but it is using a singular verb conjugation.
Also, the docs refer to $breadcrumb, which ... what is that? It isn't even a class member variable?
I think this needs a bit more work. Thanks!
Comment #15
alarcombe CreditAttribution: alarcombe commentedI attempted to address the issues in #14.
Comment #16
jhodgdonThat's better... seems to reflect what the class does. Tentatively marking RTBC unless someone else objects.
Comment #17
webchickI think this looks decent as a start. And $lies--; :) We can always improve in subsequent patches.
Committed and pushed to 8.x. Thanks!