/**
 * 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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Component: base system » documentation
Issue tags: +Novice
jhodgdon’s picture

Priority: Normal » Critical

Yeah, 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.

kgoel’s picture

Status: Active » Needs review
FileSize
615 bytes
msonnabaum’s picture

Status: Needs review » Needs work

We 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.

klausi’s picture

Priority: Critical » Normal

Why 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

jhodgdon’s picture

Classes 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.

ghazlewood’s picture

Assigned: Unassigned » ghazlewood
Status: Needs work » Needs review
FileSize
753 bytes

Here's a stab at a better description of the class, hopefully I've not got the wrong end of the stick.

klausi’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.php
@@ -8,10 +8,9 @@
- * Provides a breadcrumb manager.
+ * Creates a sorted array of breadcrumb builders and populates the $breadcrumb ¶
+ * array from the results of those breadcrumb builders in order of priority

Trailing white space, missing punctuation. And I think the first line should be a short description under 80 characters as its own paragraph.

paulh’s picture

Assigned: ghazlewood » paulh
Issue summary: View changes
Status: Needs work » Needs review
FileSize
693 bytes

Ok, 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.

paulh’s picture

Upon reflection, the description could have been a bit clearer. Updated.

paulh’s picture

Corrected for Drupal API documentation standards:

Sentences should be separated by single spaces.
taslett’s picture

Status: Needs review » Reviewed & tested by the community

@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.

paulh’s picture

Assigned: paulh » Unassigned
jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

Thanks 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!

alarcombe’s picture

I attempted to address the issues in #14.

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community

That's better... seems to reflect what the class does. Tentatively marking RTBC unless someone else objects.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I think this looks decent as a start. And $lies--; :) We can always improve in subsequent patches.

Committed and pushed to 8.x. Thanks!

  • Commit f941bbd on 8.x by webchick:
    Issue #2031485 by paulh, alarcombe, ghazlewood, kgoel, msonnabaum,...

Status: Fixed » Closed (fixed)

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