Fix issue 6904 by removing the subclass parameter from model auto complete #7851

mpoiriert · 2022-06-22T23:12:41Z

…

Subject

I am targeting this branch, because the bug was introduce here.

Closes #6904.

Changelog

### Fixed
Model Auto Complete failing in create flow of admin with subclasses

mpoiriert · 2022-06-22T23:23:49Z

@VincentLanglet Ready to review. The automation test are just checking that a subclass value is not there,. I don't think I can do more than this.

VincentLanglet · 2022-06-23T06:46:32Z

The automation test are just checking that a subclass value is not there,. I don't think I can do more than this.

Best would be a functional test to reproduce the issue from #6904. Do you think it would be possible ?

mpoiriert · 2022-06-23T08:25:52Z

The issue is base on the fact that the subclass parameter is present when we are on creation flow with an admin that have subclass calling the model auto complete endpoint via ajax. Not sure how you would proceed to do that without a panther setup ? Just calling the model auto complete endpoint without having a subclass parameter don't test anything since I didn't change anything other than removing the parameter in the twig template. The fix I made for the other issue was to remove/put back the parameter in the controller but that was a "hack" since we had too many place to check. The best solution would have been to remove the parameter completely which the only wau to test would have been to check that the parameter is not there.

…

On Thu., Jun. 23, 2022, 2:46 a.m. Vincent Langlet, ***@***.***> wrote: The automation test are just checking that a subclass value is not there,. I don't think I can do more than this. Best would be a functional test to reproduce the issue from #6904 <#6904>. Do you think it would be possible ? — Reply to this email directly, view it on GitHub <#7851 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA73OAEPTISR4C737K6EZG3VQQB5HANCNFSM5ZSGWVTA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

VincentLanglet · 2022-06-23T08:49:08Z

The fix I made for the other issue was to remove/put back the parameter in the controller but that was a "hack" since we had too many place to check. The best solution would have been to remove the parameter completely which the only wau to test would have been to check that the parameter is not there.

Does the other fix still needed then ?

mpoiriert · 2022-06-23T11:36:39Z

Yes since it's not the same endpoint.

Once we found all the subclass parameter that have issues we could revisit how the fix was done.

The current one seems to be a "final" fix for that specific use case.

franmomu · 2022-06-25T09:16:38Z

tests/Functional/Controller/CRUDControllerTest.php

+        $subclass = uniqid('subclass');
        $client = static::createClient();
-        $crawler = $client->request(Request::METHOD_GET, '/admin/tests/app/foo/create');
+        $crawler = $client->request(Request::METHOD_GET, '/admin/tests/app/foo/create?subclass='.$subclass);


IMHO this should go in a new test instead of modifying this one

Indeed @mpoiriert, two tests are better than one

tests/Functional/Controller/CRUDControllerTest.php

…plete

jordisala1991 · 2022-06-25T11:31:43Z

Thanks @mpoiriert

Geekimo · 2022-07-11T15:08:27Z

Hello,
following this PR, we have issues when editing subjects that are "abstract" and use a discriminator map.
I've found a workaround by passing the subclass parameter as req_params in ModelAutocompleteType::class.
Shall we found another way to fix @mpoiriert issue that is less "global" ?

mpoiriert · 2022-07-11T15:58:28Z

@Geekimo The issue is the way the subclass have been implemented.
What is your specific use case ? Because I removed the sublcass use from the query parameter which is conflicting. If you do an Edit the query parameter subclass is not set since it's just when you do a create.

Geekimo · 2022-07-11T18:27:33Z

@mpoiriert Our admin class can edit two entities inheriting from the same abstract entity. We found out by going back to the latest version before your fix that the subclass query parameter was missing in the ajax request, and I had to set it by hand.

mpoiriert · 2022-07-11T19:22:33Z

@Geekimo To make sure I understand you were already making a "hook" since the behaviour wasn't suppose to accomplish what you were trying to do ?

Geekimo · 2022-07-11T19:33:32Z

@mpoiriert AFAIK there wasn't any hook.
Maybe it has never worked for edit and nobody noticed it . Strange.
Thanks for the reply.

jordisala1991 · 2022-07-11T20:48:06Z

Not sure if I follow correctly the conversation. Something broke after this PR?

mpoiriert · 2022-10-11T08:16:47Z

It's not the same endpoint. So yes. Once we found all the subclass parameter that have issues we could revisit how the fix was done. I'll check this later and let you know.

…

On Thu., Jun. 23, 2022, 4:49 a.m. Vincent Langlet, ***@***.***> wrote: The fix I made for the other issue was to remove/put back the parameter in the controller but that was a "hack" since we had too many place to check. The best solution would have been to remove the parameter completely which the only wau to test would have been to check that the parameter is not there. Does the other fix still needed then ? — Reply to this email directly, view it on GitHub <#7851 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA73OAEJGZIZ45HR4QA53IDVQQQJBANCNFSM5ZSGWVTA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

ENDaZONELT · 2023-10-13T11:40:03Z

@mpoiriert any news regarding this bug ? I have the same issue. Abstract entity and discriminator map. I need to pass subclass parameter manually via req_params:
'req_params' => ['subclass' => array_search(get_class($this->getSubject()), $this->getSubClasses())],

mpoiriert · 2023-10-13T12:18:28Z

@ENDaZONELT The issue was that subclass should not be passed automatically. I don't know your exact use case but the one that I fix was because AdminA (wiht subclass) -> Calling AutoComplete of AdminB was not working because the subclass of admin A was pass to AdminB.

mpoiriert force-pushed the issue-6904-model-auto-complete branch 2 times, most recently from b1a4467 to b0955d4 Compare June 22, 2022 23:16

VincentLanglet requested review from a team and jordisala1991 June 23, 2022 08:49

VincentLanglet added the patch label Jun 23, 2022

VincentLanglet previously approved these changes Jun 23, 2022

View reviewed changes

VincentLanglet requested a review from a team June 23, 2022 11:46

franmomu reviewed Jun 25, 2022

View reviewed changes

mpoiriert dismissed VincentLanglet’s stale review via 3ace714 June 25, 2022 10:54

mpoiriert force-pushed the issue-6904-model-auto-complete branch from b0955d4 to 3ace714 Compare June 25, 2022 10:54

VincentLanglet previously approved these changes Jun 25, 2022

View reviewed changes

VincentLanglet reviewed Jun 25, 2022

View reviewed changes

tests/Functional/Controller/CRUDControllerTest.php Outdated Show resolved Hide resolved

Fix issue 6904 by removing the subclass parameter from model auto com…

3acf85f

…plete

mpoiriert dismissed VincentLanglet’s stale review via 3acf85f June 25, 2022 11:03

mpoiriert force-pushed the issue-6904-model-auto-complete branch from 3ace714 to 3acf85f Compare June 25, 2022 11:03

VincentLanglet approved these changes Jun 25, 2022

View reviewed changes

VincentLanglet mentioned this pull request Jun 25, 2022

Improve lazyness of commands #7854

Merged

jordisala1991 approved these changes Jun 25, 2022

View reviewed changes

jordisala1991 merged commit c362499 into sonata-project:4.x Jun 25, 2022

mpoiriert deleted the issue-6904-model-auto-complete branch December 31, 2023 23:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue 6904 by removing the subclass parameter from model auto complete #7851

Fix issue 6904 by removing the subclass parameter from model auto complete #7851

mpoiriert commented Jun 22, 2022

mpoiriert commented Jun 22, 2022

VincentLanglet commented Jun 23, 2022 •

edited

Loading

mpoiriert commented Jun 23, 2022 via email

VincentLanglet commented Jun 23, 2022

mpoiriert commented Jun 23, 2022

franmomu Jun 25, 2022

VincentLanglet Jun 25, 2022

mpoiriert Jun 25, 2022

jordisala1991 commented Jun 25, 2022

Geekimo commented Jul 11, 2022

mpoiriert commented Jul 11, 2022

Geekimo commented Jul 11, 2022

mpoiriert commented Jul 11, 2022

Geekimo commented Jul 11, 2022 •

edited

Loading

jordisala1991 commented Jul 11, 2022

mpoiriert commented Oct 11, 2022 via email

ENDaZONELT commented Oct 13, 2023

mpoiriert commented Oct 13, 2023

Fix issue 6904 by removing the subclass parameter from model auto complete #7851

Fix issue 6904 by removing the subclass parameter from model auto complete #7851

Conversation

mpoiriert commented Jun 22, 2022

Subject

Changelog

mpoiriert commented Jun 22, 2022

VincentLanglet commented Jun 23, 2022 • edited Loading

mpoiriert commented Jun 23, 2022 via email

VincentLanglet commented Jun 23, 2022

mpoiriert commented Jun 23, 2022

franmomu Jun 25, 2022

Choose a reason for hiding this comment

VincentLanglet Jun 25, 2022

Choose a reason for hiding this comment

mpoiriert Jun 25, 2022

Choose a reason for hiding this comment

jordisala1991 commented Jun 25, 2022

Geekimo commented Jul 11, 2022

mpoiriert commented Jul 11, 2022

Geekimo commented Jul 11, 2022

mpoiriert commented Jul 11, 2022

Geekimo commented Jul 11, 2022 • edited Loading

jordisala1991 commented Jul 11, 2022

mpoiriert commented Oct 11, 2022 via email

ENDaZONELT commented Oct 13, 2023

mpoiriert commented Oct 13, 2023

VincentLanglet commented Jun 23, 2022 •

edited

Loading

Geekimo commented Jul 11, 2022 •

edited

Loading