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

Files: 
CommentFileSizeAuthor
#9 breadcrumb-2099095-9.patch17.85 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,952 pass(es).
[ View ]
#6 drupal8.base-system.2099095-6.patch17.87 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,671 pass(es).
[ View ]
#6 interdiff.txt3.37 KBjibran
#3 drupal8.base-system.2099095-3.patch17.97 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 59,053 pass(es).
[ View ]
BreadcrumbBuilderBase.patch17.66 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,694 pass(es).
[ View ]

Comments

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

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

Working on the reroll

Assigned:jibran» Unassigned
Status:Needs work» Reviewed & tested by the community
StatusFileSize
new17.97 KB
PASSED: [[SimpleTest]]: [MySQL] 59,053 pass(es).
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new3.37 KB
new17.87 KB
PASSED: [[SimpleTest]]: [MySQL] 58,671 pass(es).
[ View ]

@Crell Nice catch. Fixed #5

Status:Needs review» Reviewed & tested by the community

Forward!

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

Patch no longer applies.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new17.85 KB
PASSED: [[SimpleTest]]: [MySQL] 58,952 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Back to RTBC

Title:Create BreadcrumbBuilderBase and remove the boilerplate code from BreadcrumbBuildersChange 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.

Title:Change notice: Create BreadcrumbBuilderBase and remove the boilerplate code from BreadcrumbBuildersCreate 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 :).

Priority:Major» Normal

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