Access manager service seems not to be used there.

https://api.drupal.org/api/drupal/core%21modules%21book%21src%21BookBrea...

CommentFileSizeAuthor
#2 2681903-2.patch2.07 KBChi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Chi created an issue. See original summary.

Chi’s picture

Chi CreditAttribution: Chi commented
Status: Active » Needs review
FileSize
2681903-2.patch2.07 KB
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ commented

I think this is an API break and that means we can't do this before 9.x

Chi’s picture

Chi CreditAttribution: Chi commented

@borisson_, how can this change break anything?

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ commented

Changing the signature of a public method on a class that's not marked @internal changes the public API. I think that's something we're not allowed to do in 8.x

Chi’s picture

Chi CreditAttribution: Chi commented

The service should be always instantiated through the service container.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ commented
Status: Needs review » Reviewed & tested by the community

I wasn't sure, so I looked it up, according to #2550249: [meta] Document @internal APIs both explicitly in phpdoc and implicitly in d.o documentation - this change is allowed. So setting this to RTBC.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

  • catch committed f04c292 on 8.2.x 
    Issue #2681903 by Chi: Unused dependency in Book breadcrumb builder...

  • catch committed bdca52e on 8.1.x 
    Issue #2681903 by Chi: Unused dependency in Book breadcrumb builder...

Status: Fixed » Closed (fixed)

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