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 shared block service #1618

Merged

Conversation

jordisala1991
Copy link
Member

@jordisala1991 jordisala1991 commented Sep 26, 2022

Subject

I am targeting this branch, because shared blocks are broken on 4.x.

Fixes: #1495 (comment)

@jordisala1991
Copy link
Member Author

This issue is hard to test because it involves the need of a panther test.

@jordisala1991 jordisala1991 force-pushed the hotfix/fix-shared-blocks branch from c8bab66 to b7a97a1 Compare September 26, 2022 14:30
@jordisala1991
Copy link
Member Author

@eerison on my tests, with these changes, the shared block works fine, can you try this?

@@ -130,20 +139,17 @@ public function load(BlockInterface $block): void
*/
private function getBlockBuilder(AdminFormMapper $form): FormBuilderInterface
{
$admin = $form->getAdmin();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't work with the admin coming from the form ?

Copy link
Member Author

@jordisala1991 jordisala1991 Sep 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you use the admin from the form, you are using BlockAdmin, but this field is to related with SharedBlockAdmin. What is more interesting is that this is happening inside PageAdmin, so for some reason, not only the admin linked is not the correct one which shows Blocks, not shared blocks. BUT also Sonata is smart enough to know Block is child of Page and automatically builds child urls, showing only blocks related to that page. And the final funnier thing is that for child urls , the select button does not work, I can't understand why tho.

So to answer you:

  1. No, the admin is not BlockAdmin but SharedBlockAdmin
  2. Previously only blocks belonging to the page were shown (instead of the expected behavior which is showing ALL shared blocks)
  3. Sonata has a bug somewhere (I didn't fully understand why) that make the previous implementation fail because it tries to open a modal with a child, but without providing the correct linkParameters (it is hard to explain because I do not fully know what is happening there)
  4. But the bug pointed on 3. is not really related to this issue, which is: We were relating to block instead of shared block.

Maybe we want to investigate more about this 3. in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or explaining the other way around: If the point 3 didn't exists, this block should work, but it will let you only blocks that are already present on the page, not shared block intended to be reused across pages.

@eerison
Copy link
Contributor

eerison commented Sep 27, 2022

Hey @jordisala1991 I tested your branch and I got the same behaviour,

I create the block this way 👇🏼

Screen.Recording.2022-09-27.at.09.41.09.mov

@jordisala1991
Copy link
Member Author

I think you are testing another thing, this fixes the shared block service (adding a shared block inside a page), not the batch delete.

@eerison
Copy link
Contributor

eerison commented Sep 27, 2022

ahhh ok! I got it :)

let me check again

@eerison
Copy link
Contributor

eerison commented Sep 27, 2022

Hey @jordisala1991

now I can add a share block in my page ❤️

@jordisala1991 jordisala1991 merged commit 720413c into sonata-project:4.x Sep 27, 2022
@jordisala1991 jordisala1991 deleted the hotfix/fix-shared-blocks branch September 27, 2022 07:49
@eerison eerison mentioned this pull request Sep 27, 2022
36 tasks
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.

3 participants