Updated: Comment #0

Problem/Motivation

Create BreadcrumbBuilderBase and remove the boilerplate code from BreadcrumbBuilders

Proposed resolution

Create BreadcrumbBuilderBase and remove the boilerplate code from BreadcrumbBuilders

Remaining tasks

Review/RTBC the patch

User interface changes

None

API changes

Clean up

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Just like that.
I can't fault it.
Can this be added to the DX initiative meta (if not already there).

jibran’s picture

Assigned: Unassigned » jibran
Status: Reviewed & tested by the community » Needs work

Working on the reroll

jibran’s picture

Assigned: jibran » Unassigned
Status: Needs work » Reviewed & tested by the community
FileSize
17.97 KB
Crell’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/book/lib/Drupal/book/BookBreadcrumbBuilder.php
@@ -8,16 +8,15 @@
-class BookBreadcrumbBuilder implements BreadcrumbBuilderInterface {
+class BookBreadcrumbBuilder extends BreadcrumbBuilderBase implements BreadcrumbBuilderInterface {

Since BreadcrumbBuilderBase already implements BreadcrumbBuilderInterface, that's not needed on any of the subclasses.

Otherwise, this looks consistent with what we discussed in Prague so +1. (Although I'd still prefer to include the setters for test overriding on the base classes to make them easier to use; we can very easily document that they're for testing purposes only.)

jibran’s picture

Status: Needs work » Needs review
FileSize
3.37 KB
17.87 KB

@Crell Nice catch. Fixed #5

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Forward!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
17.85 KB

Someone else edited comment.services.yml. No change.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

catch’s picture

Title: Create BreadcrumbBuilderBase and remove the boilerplate code from BreadcrumbBuilders » Change notice: Create BreadcrumbBuilderBase and remove the boilerplate code from BreadcrumbBuilders
Priority: Normal » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Looks great. Committed/pushed to 8.x, thanks!

Could use a change notice for the API addition.

jibran’s picture

Title: Change notice: Create BreadcrumbBuilderBase and remove the boilerplate code from BreadcrumbBuilders » Create BreadcrumbBuilderBase and remove the boilerplate code from BreadcrumbBuilders
Status: Active » Fixed
Issue tags: -Needs change record

Updated https://drupal.org/node/2026025/revisions/view/2855137/2868077. Added https://drupal.org/node/2106757.
PS: I have created the issue and marked it as fixed. Thanks everybody :).

jibran’s picture

Priority: Major » Normal

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