subclass parameter conflict between admin

[mpoiriert]

There is name conflict for the subclass parameter.

If you are in the creating flow (since the on edit or show the subclass parameter don't exits) and one of the form type that need to call another admin (Example ModelListType) that has subclass as well it will failed. The subclass parameter from the current create admin will be pass to the ajax call (to load the information) and the other admin will failed since the class referred by the subclass parameter will not exists.

Solution would be to prefix the parameter name with the admin code (or something the sort) that way the parameter will be only interpreted by the proper admin.

This would cause issue to people that are generated url with subclass outside of the admin. We could trigger a deprecated if present and use it any way. It would not solve the issue of people using $request->query->get('subclass') directly.

We could make a configuration that need to be explicitly enable to fix the issue.

This is present in v3 and v4

[VincentLanglet]

I'm not sure to understand. Can you point out in the code which subclass are in conflict ? https://github.com/sonata-project/SonataAdminBundle/search?q=subclass

I'm ok to introduce new name for some of them since this would be BC if we do $request->get('newName', $request->get('oldName')).

[mpoiriert]

This is a code sample to explain the model configuration and the admin configuration.

abstract class BaseClassXYZ
{
}

class ClassA extends BaseClassXYZ
{
}

class ClassB extends BaseClassXYZ
{
}

abstract class BaseClass123
{
}

class Class1 extends BaseClass123
{
}

class Class2 extends BaseClass123
{
    public BaseClassXYZ $manyToOne;
}


class BaseClassXYZAdmin extends \Sonata\AdminBundle\Admin\AbstractAdmin
{
    public function __construct()
    {
        $this->setSubClasses([
            'ClassA' => ClassA::class,
            'ClassB' => ClassB::class,
        ]);
        parent::__construct();
    }
}

class BaseClass123Admin extends \Sonata\AdminBundle\Admin\AbstractAdmin
{
    public function __construct()
    {
        $this->setSubClasses([
            'Class1' => Class1::class,
            'Class2' => Class2::class,
        ]);
        parent::__construct();
    }

    protected function configureFormFields(\Sonata\AdminBundle\Form\FormMapper $form): void
    {
        $form
            ->add('manyToOne', \Sonata\AdminBundle\Form\Type\ModelType::class);
    }
}

If you try to create a new BaseClass123 via the admin after selecting the subclass you will reach a URL like this:

/baseclassadmin123/create?subclass=Class1

If you use the form component to select the manyToOne relation (via the list) after, to load the information, a ajax call will be done to a url of the sort url:

/admin/core/get-short-object-description?_sonata_admin=BaseClassXYZAdmin&objectId=1&uniqid=s62a36876b2a7b&_sonata_name=admin_baseclassadmin123_create&subclass=Class1

As you can see the original query parameter are kept (AKA the subclass=Class1).

When reaching the other admin BaseClassXYZAdmin We will get an exception because of the code

SonataAdminBundle/src/Admin/AbstractAdmin.php

Line 650 in f9d6325

throw new \LogicException(sprintf('Subclass "%s" is not defined.', $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);       

This is because the subclass parameter is not bind to a specific admin and since all the request parameter are forwarded to the ajax call, if you are in the creation flow everything that request something from an admin that also have SubClases it will failed.

There is other flow affected by this I think but I don't remember which.

This is why I suggest the parameter to be something like {$adminCode}_subsclass to prevent conflict but it will cause issue as explained in the issue description.

I could have resolve the issue with a temporary fix but since everything is now final everywhere it make this really complicated.

[VincentLanglet]

I see. If the only way to solve this issue is to introduce a BC-break, I think it can be ok (wdyt @jordisala1991).

We can do

/baseclassadmin123/create?prefixSubclass=Class1

but keeping

$subClass = $this->getRequest()->query->get('prefixSubclass', $this->getRequest()->query->get('subclass'));

This will reduce the BC break

(with prefix being the dynamic value of course)

[mpoiriert]

The only thing that will not be backward compatible with your solution is the people that are doing $request->query->get('subclass') by themself.

We should provide a method to get the new subsclass parameter via the admin.

Since the create admin is probably the only place a subclass parameter is required would it make sens to make it a optional path parameter via the RouteCollection if the admin has a subclass ? I am not sure how to hook this automatically but it should be possible. If we do this it would solve the issue of prefixing since it will not be a query parameter so it will not get forwarded. It will not be backward compatible but might be a big cleaner and easier to integrate at the end.

[VincentLanglet]

The only thing that will not be backward compatible with your solution is the people that are doing $request->query->get('subclass') by themself.

Since I don't see any other easy solution for this issue, I would be ok with such a BC break.

We should provide a method to get the new subsclass parameter via the admin.

I'd like to avoid adding extra method to the AdminInterface, and I don't know elsewhere we could add it.
I can be introduce but I don't think it's mandatory.

Since the create admin is probably the only place a subclass parameter is required would it make sens to make it a optional path parameter via the RouteCollection if the admin has a subclass ? I am not sure how to hook this automatically but it should be possible. If we do this it would solve the issue of prefixing since it will not be a query parameter so it will not get forwarded. It will not be backward compatible but might be a big cleaner and easier to integrate at the end.

I dunno if it's possible, and how, but it could be interesting.

[mpoiriert]

I have check how to do it with the collection. What I suggest is to put a configuration something like: sonata_admin.none_bc_fix.subclass_as_path_argument: (true|false) default false. If true I will decorate the collection builder and append the subclass in the create path, only if the admin has subclasses. Changing the code for $request->attributes->get('subclass', $request->query->get('subclass') like you suggested. People that are using $admin->getSubClassCode() will not have any issues. Only issue is for people that are bypassing admin method and are using the subclass query parameters directly. We can add a few deprecation warming as well. For the config not being set to true and if the subclass query parameter is present. In next major we will only check via the path parameter and will ignore the configuration. A deprecation warming stating that configuration is ignored if defined and need to be removed for next major (aka V6). Or we ignore all deprecation and expect people to use only the methods in the admin to handle subclasses. I think if they did use the provided methods everything should work as expected. I do have some test in my projects that call the url that I have hard-coded (since I want to be aware of url change) but this is not a documented usage of the library.

…

On Sun., Jun. 12, 2022, 4:40 a.m. Vincent Langlet, ***@***.***> wrote: The only thing that will not be backward compatible with your solution is the people that are doing $request->query->get('subclass') by themself. Since I don't see any other easy solution for this issue, I would be ok with such a BC break. We should provide a method to get the new subsclass parameter via the admin. I'd like to avoid adding extra method to the AdminInterface, and I don't know elsewhere we could add it. I can be introduce but I don't think it's mandatory. Since the create admin is probably the only place a subclass parameter is required would it make sens to make it a optional path parameter via the RouteCollection if the admin has a subclass ? I am not sure how to hook this automatically but it should be possible. If we do this it would solve the issue of prefixing since it will not be a query parameter so it will not get forwarded. It will not be backward compatible but might be a big cleaner and easier to integrate at the end. I dunno if it's possible, and how, but it could be interesting. — Reply to this email directly, view it on GitHub <#7843 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA73OAFSXJ6VY7HM6S55LNLVOWO6RANCNFSM5YKTF5OQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[mpoiriert]

I have tested and we can put the subclass as an optional parameter in the route path.

Unfortunately because of this:

  jQuery.ajax({
                type: 'GET',
                url: '{{ path('sonata_admin_short_object_information',{
                    '_sonata_admin': associationadmin.baseCodeRoute,
                    'objectId': 'OBJECT_ID',
                    'uniqid': associationadmin.uniqid,
                    'linkParameters': sonata_admin.field_description.option('link_parameters', {})
                } + (
                    associationadmin.hasRequest()
                    ? associationadmin.request.attributes.get('_route_params', {})
                    : {}
                ) + app.request.query.all|default({})
                )}}'.replace('OBJECT_ID', objectId),
                dataType: 'html',
                success: function(html) {
                    jQuery('#field_widget_{{ id }}').html(html);
                }
            });

We can see the _route_params are also forwarded an it does include the path parameter.

So we are at the same point.

If we remove backward compatibility (which is not complete anyway) and not use

 $request->attributes->get('subclass', $request->query->get('subclass'));

But only

 $request->attributes->get('subclass');

We are good to go.

If we want partial backward compatibility we will need to prefix the parameter.

[VincentLanglet]

I would recommend to start a PR with this, it will be easier to discuss about it (and get some others opinions).
But if we keep all the existing features, and fix a bug, I would be ok with the well-documented BC-break.

[mpoiriert]

There is so many place the subclass is use and so many place that all parameter are just forwarded it's almost impossible to fix properly the way it have been implemented.

I am not confident that there is enough code coverage to be sure that everything will be working.

Didn't find any solution so far I am checking how to fix my current use case specifically. It might be the only way.

[VincentLanglet]

I am not confident that there is enough code coverage to be sure that everything will be working.

Would you be more confident with the admin prefix ?

[mpoiriert]

I am not confident that there is enough code coverage to be sure that everything will be working.

Would you be more confident with the admin prefix ?

Same thing with the prefix since all subclass parameter will need to be change and we don't want to expose a new method in the admin.

I think the fact that we are forwarding everything even if it's not needed is the issue here. But changing this will cause even more BK-break.