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

CommentFileSizeAuthor
#9 breadcrumb-2099095-9.patch17.85 KBtim.plunkett
#6 drupal8.base-system.2099095-6.patch17.87 KBjibran
#6 interdiff.txt3.37 KBjibran
#3 drupal8.base-system.2099095-3.patch17.97 KBjibran
BreadcrumbBuilderBase.patch17.66 KBjibran

Comments

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
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

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Assigned: Unassigned » jibran
Status: Reviewed & tested by the community » Needs work

Working on the reroll

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Assigned: jibran » Unassigned
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new drupal8.base-system.2099095-3.patch17.97 KB

Reroll after #731724: Convert comment settings into a field to make them work with CMI and non-node entities.

Crell’s picture

Crell commented
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

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt3.37 KB
new drupal8.base-system.2099095-6.patch17.87 KB

@Crell Nice catch. Fixed #5

Crell’s picture

Crell commented
Status: Needs review » Reviewed & tested by the community

Forward!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new breadcrumb-2099095-9.patch17.85 KB

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

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC

catch’s picture

catch
he/him
Primary language English
commented
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

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
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

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Priority: Major » Normal

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