Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix UserAdmin breadcrumb name #1533

Merged

Conversation

aerrasti
Copy link
Contributor

@aerrasti aerrasti commented Apr 29, 2022

Subject

The breadcrumb builder is using a classnameLabel that doesn't match the translationbreadcrumb.link_user_list.

Before
image

After
image

I am targeting this branch, because the error is in the 5.x branch.

Changelog

### Changed
- Changed UserAdmin classnameLabel. This affects the breadcrumb translation generation. With this change Sonata will always pick the SonataUserBundle breadcrumbs translations by default.

@jordisala1991
Copy link
Member

I think we might want to investigate a bit. On Sonata 3 (With UserBundle 4) this problem didn't happened. Do you know if something changes between Sonata 3 and 4 that might be related @VincentLanglet ?

@VincentLanglet
Copy link
Member

I think we might want to investigate a bit. On Sonata 3 (With UserBundle 4) this problem didn't happened. Do you know if something changes between Sonata 3 and 4 that might be related @VincentLanglet ?

Dunno.

But looking at the fix, it would meant that the classname label is not correctly set.
Currently, it's done this way.

final public function initialize(): void
    {
        if (null === $this->classnameLabel) {
            $namespaceSeparatorPos = strrpos($this->getClass(), '\\');
            $this->classnameLabel = false !== $namespaceSeparatorPos
                ? substr($this->getClass(), $namespaceSeparatorPos + 1)
                : $this->getClass();
        }

        $this->configure();

        foreach ($this->getExtensions() as $extension) {
            $extension->configure($this);
        }
    }

and getClass

final public function getClass(): string
    {
        if ($this->hasActiveSubClass()) {
            if ($this->hasParentFieldDescription()) {
                throw new \LogicException('Feature not implemented: an embedded admin cannot have subclass');
            }

            $subClass = $this->getRequest()->query->get('subclass');
            \assert(\is_string($subClass));

            if (!$this->hasSubClass($subClass)) {
                throw new \LogicException(sprintf('Subclass "%s" is not defined.', $subClass));
            }

            return $this->getSubClass($subClass);
        }

        // Do not use `$this->hasSubject()` and `$this->getSubject()` here to avoid infinite loop.
        // `getSubject` use `hasSubject()` which use `getObject()` which use `getClass()`.
        if (null !== $this->subject) {
            /** @phpstan-var class-string<T> $class */
            $class = ClassUtils::getClass($this->subject);

            return $class;
        }

        return $this->getModelClass();
    }

It would be interesting to dump the value to understand where is the issue.

The modelClass is set here
https://github.com/sonata-project/SonataUserBundle/blob/5.x/src/Resources/config/admin_orm.php#L23

Coming from https://github.com/sonata-project/SonataUserBundle/blob/5.x/src/DependencyInjection/SonataUserExtension.php#L90

@aerrasti What's the name of your user class ?
@jordisala1991 I think if the User class is not called User you'll end with this issue in the breadcrumb.

@jordisala1991
Copy link
Member

Hey I just checked and it happens the same with Sonata 3, so it's not something related to the upgrade.

@VincentLanglet
Copy link
Member

Hey I just checked and it happens the same with Sonata 3, so it's not something related to the upgrade.

What's the user class you used ? Do I suppose correctly the origin of the bug ?

@jordisala1991
Copy link
Member

jordisala1991 commented Apr 29, 2022

To me the problem is that, by default we add some translations for the breadcrumbs and all. But at the end, it depends if the class defined by the developer is called User or not. If it is called SonataUser or any other thing, the translation won't be used.

@VincentLanglet
Copy link
Member

To me the problem is that, by default we add some translations for the breadcrumbs and all. But at the end, it depends if the class defined by the developer is called User or not. If it is called SonataUser or any other thing, the translation won't be used.

Yes, that's why the fix is correct.
But if someone used a custom translations breadcrumb.link_sonata_user_user_list, we will break his code with this patch.

Should it be a minor with a upgrade note "be careful" ?

@jordisala1991
Copy link
Member

Yes, we can consider it a minor. At worst, people will see their translations changed and used now the default of Sonata, which TBH is not that big of a deal, but we can give them the hint.

@jordisala1991 jordisala1991 added minor and removed patch labels Apr 29, 2022
@VincentLanglet VincentLanglet merged commit 456b952 into sonata-project:5.x Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants