Problem/Motivation

Once #2313309: Admin toolbar should always be rendered in the admin language (if set) lands, we would need to update the Drupal\login_destination\LoginDestinationToolbarLinkBuilder constructor. Let's stop extending the decorated class so we don't have to update constructors in the future.

Steps to reproduce

Visit any page showing the toolbar with a patch from #2313309: Admin toolbar should always be rendered in the admin language (if set) applied.

Proposed resolution

Stop extending the decorated class.

CommentFileSizeAuthor
#4 3261748-4.patch1.82 KBdieterholvoet
#3 3261748-3.patch1.2 KBdieterholvoet

Issue fork login_destination-3261748

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

DieterHolvoet created an issue. See original summary.

DieterHolvoet opened merge request !1

dieterholvoet’s picture

dieterholvoet commented
StatusFileSize
new 3261748-3.patch1.2 KB

I attached a patch of the current state of the MR for easy inclusion in projects.

dieterholvoet’s picture

dieterholvoet commented
StatusFileSize
new 3261748-4.patch1.82 KB
parisek’s picture

parisek
Primary language Czech
commented

@DieterHolvoet there is better way to fix this

login_destination implements a decorator pattern, but in the wrong way. It is completely up to contrib to make sure the implementations are correct.

Side information: decorators should not extend the class they're decorating. They should implement the correct interface, implement all methods and use $this->inner->method calls for each method they're not decorating. This mistake is made often though, but it shouldn't block improvements from going in core imo.

https://www.drupal.org/project/drupal/issues/2313309#comment-14188467

dieterholvoet’s picture

dieterholvoet commented
Title: Admin toolbar should always be rendered in the admin language (if set) » LoginDestinationToolbarLinkBuilder implements the decorator pattern in a wrong way
Issue summary: View changes
Status: Postponed » Needs review

Good catch! I updated the PR, this doesn't need to be postponed anymore.

parisek’s picture

parisek
Primary language Czech
commented
Status: Needs review » Reviewed & tested by the community

Works for me on several projects for months - RTBC

Thank you

rsvelko made their first commit to this issue’s fork.

rsvelko’s picture

rsvelko commented
Status: Reviewed & tested by the community » Fixed

updated the merge request branch, then merged PR/MR.
Tada!

Status: Fixed » Closed (fixed)

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