Prepare for BlockBundle 4.0 support and fix #1731 #1732

haivala · 2020-04-06T06:43:30Z

Subject

I am targeting this branch, because it is patch.

Changelog

### Added
- prepare for BlockBundle 4.0 Support with `EditableBlockService`
### Fixed
- Added `GalleryBlockService::buildCreateForm` and `GalleryBlockService::validateBlock` to fix compatibility with PageBundle
- Added `GalleryListBlockService::buildCreateForm` and `GalleryListBlockService::validateBlock` to fix compatibility with PageBundle
- Added `MediaBlockService::buildCreateForm` and `MediaBlockService::validateBlock` to fix compatibility with PageBundle

src/Block/GalleryBlockService.php

phansys

I guess these extension points are intended to be kept in the next major.

haivala · 2020-04-06T11:57:47Z

This will fix only display Block in 3.x version. To full bugfix please do:

* copy methods from `Sonata/BlockBundle/Block/Service/AbstractAdminBlockService`

* add deprecation to them like AbstractAdmin.. have

* add `implements EditableBlockServie` and connect news methods with existing

I'll check this later

haivala · 2020-04-06T13:21:28Z

With these changes I can now add block in the Page composer but it still needs the validate method to be able to create the block.
Is it good if I add?

use Sonata\Form\Validator\ErrorElement;
...
    public function validateBlock(ErrorElement $errorElement, BlockInterface $block)
    {
    }

I tested and after that I can create the block.
Should I add that to this pull request?

phansys · 2020-04-07T17:10:50Z

Is not clear for me if these methods are added back just to fix a BC break (and must be deprecated), or if they are intended to be kept in the next major. Could you please confirm that?

wbloszyk · 2020-04-07T17:56:07Z

This commit make bug: 9533247 becouse AbstractAdminBlockService without add EditableBlockService
Look at: SonataPageBundle/blob/3.x/src/Admin/BlockAdmin.php#L217-L238

To fix it and keep BC-break all methods from AbstractAdminBlockService must be implemented and mark as depecated and NEXT_MAJOR: delete it to keep BlockBundle 3 support.

To add BlockBundle 4 support EditableBlockService must be implemented.

phansys · 2020-04-07T18:29:49Z

Thank you for the answer.

To fix it and keep BC-break all methods from AbstractAdminBlockService must be implemented and mark as depecated and NEXT_MAJOR: delete it to keep BlockBundle 3 support.

To add BlockBundle 4 support EditableBlockService must be implemented.

IIUC, I think we should add a NEXT_MAJOR comments, telling these methods must be removed and the interface EditableBlockService must be implemented here:

SonataMediaBundle/src/Block/GalleryBlockService.php

Lines 40 to 45 in 4be6b0f

	/**
	* @final since sonata-project/media-bundle 3.21.0
	*
	* @author Thomas Rabaix <[email protected]>
	*/
	class GalleryBlockService extends AbstractBlockService

haivala · 2020-04-08T07:03:22Z

How do I solve this: "Attempted to call an undefined method named "getAdmin" of class "Sonata\PageBundle\Mapper\PageFormMapper"."
If comes from:

SonataMediaBundle/src/Block/MediaBlockService.php

Line 283 in 7df47c0

$fieldDescription->setAdmin($formMapper->getAdmin());

haivala · 2020-04-08T07:26:33Z

And is it ok to remove the $code variable in validate method like I have done?

haivala · 2020-04-08T09:49:49Z

If this goes smoothly: now that I have experience on this I can consider adding BlockBundle 4 support other SonataBundle blocks too.

haivala · 2020-04-08T16:37:49Z

Can I have comment on this? @wbloszyk or @phansys ?

src/Block/GalleryBlockService.php

haivala · 2020-04-08T17:53:34Z

How do I solve this: "Attempted to call an undefined method named "getAdmin" of class "Sonata\PageBundle\Mapper\PageFormMapper"."
If comes from:

SonataMediaBundle/src/Block/MediaBlockService.php

Line 283 in 7df47c0

$fieldDescription->setAdmin($formMapper->getAdmin());

@phansys this is the real problem

phansys · 2020-04-08T18:52:03Z

How do I solve this: "Attempted to call an undefined method named "getAdmin" of class "Sonata\PageBundle\Mapper\PageFormMapper"."

I didn't dive in this issue, but maybe you could use a class property in order to store the required admin instance. Otherwise, you should implement a solution based on func_get_args() in order to keep the method signature while allowing it to receive an extra argument. What I really don't know is who is responsible to provide that admin instance.

haivala · 2020-04-08T19:37:16Z

How do I solve this: "Attempted to call an undefined method named "getAdmin" of class "Sonata\PageBundle\Mapper\PageFormMapper"."

I didn't dive in this issue, but maybe you could use a class property in order to store the required admin instance. Otherwise, you should implement a solution based on func_get_args() in order to keep the method signature while allowing it to receive an extra argument. What I really don't know is who is responsible to provide that admin instance.

How I understand this is that we just need to provide the current admin (sonata.page.admin.block in this case) to it so that it can create the virtual ManyToOne relation in ImmutableArrayType form. The block admin saves the id of an Media as int in the block settings and loads the object when needed from media admin but we need to create that that form to be able to set the int (and use media admin to do so). This has been the implementation for this more than 5 years and I have feeling that it has always been considered as hacky.

My question is can I just get the sonata.page.admin.block from the container in that line? Does it beak something else? I tested this and it works as it should. like this:
$fieldDescription->setAdmin($this->container->get('sonata.page.admin.block'));

…onata.page.admin.block')

haivala · 2020-04-09T08:35:19Z

src/Block/GalleryListBlockService.php

    public function getBlockMetadata($code = null)
    {
+        if ('sonata_deprecation_mute' !== (\func_get_args()[2] ?? null)) {


Does this 2 need to be changed to 1?

It depends on how are you providing the arguments for this method, but since there is no magic usage for argument 2 (offset 1 for func_get_args()), I think you must use it.

src/Block/MediaBlockService.php

haivala · 2020-04-09T09:10:40Z

@phansys waiting for review

wbloszyk · 2020-04-08T07:28:01Z

src/Block/GalleryBlockService.php

            'class' => 'fa fa-picture-o',
        ]);
    }

+    /**
+     * NEXT_MAJOR: delete it.


Suggested change

* NEXT_MAJOR: delete it.

* NEXT_MAJOR: delete it.

*

* @deprecated Will be removed in version 4.0. Use getMetadata instead.

It triggers deprecation notice already..

wbloszyk · 2020-04-09T09:31:15Z

src/Block/FeatureMediaBlockService.php

@@ -13,7 +13,7 @@

 namespace Sonata\MediaBundle\Block;

-use Sonata\AdminBundle\Form\FormMapper;
+use Sonata\BlockBundle\Form\Mapper\FormMapper;


Suggested change

use Sonata\BlockBundle\Form\Mapper\FormMapper;

use Sonata\AdminBundle\Form\Mapper\FormMapper as AdminFormMapper;

use Sonata\BlockBundle\Form\Mapper\FormMapper;

Use AdminFormMapper in buildCreateForm and buildEditForm
Use FormMapper in configureCreateForm and configureEditForm

This can make some problem, I will check it.

wbloszyk · 2020-04-09T09:34:10Z

src/Block/GalleryBlockService.php

@@ -140,7 +145,7 @@ public function buildEditForm(FormMapper $formMapper, BlockInterface $block)
            'translation_domain' => 'SonataMediaBundle',
        ]);
        $fieldDescription->setAssociationAdmin($this->getGalleryAdmin());
-        $fieldDescription->setAdmin($formMapper->getAdmin());
+        $fieldDescription->setAdmin($this->container->get('sonata.page.admin.block'));


It is OK for old Symfony practice.

For new pratice we should avoid using container. Please move it this service to block property and set it in constructor. This will allow inject required servaices insted container.

I think the container should be removed in next major not in this patch. The container is used else where in the blocks too for example when we are getting the media admin. Can we have another pull request for that?

Container should be remove in Block 4 support becouse you must change constructor parameters.

Well this can be changed to be "prepare for Block 4 support".
I would happily do this later.
This was supposed to be patch for these blocks to work at all and now they work.
OK?

wbloszyk · 2020-04-09T14:37:58Z

src/Block/MediaBlockService.php

+                static::class
+            ), E_USER_DEPRECATED);
+        }
+        $this->configureEditForm($formMapper, $block);


FormMapper are not the same:

public function buildEditForm(Sonata\AdminBundle\Form\FormMapper $form, BlockInterface $block) public function configureEditForm(Sonata\BlockBundle\Form\Mapper\FormMapper $form, BlockInterface $block)

You should use something like PageFormMapper:

$blockMapper = new PageFormMapper($formMapper);

Sonata\BlockBundle\Form\Mapper\FormMapper has been introduced in block bundle 3.13
And this requires 3.17: https://github.com/sonata-project/SonataMediaBundle/blob/3.x/composer.json#L66
block bundles formmapper uses the same adminbundle formmapper without some functions.
Is this really needed?

The thing is that when I introduced the BC break in #1699, it was extending AbstractAdminBlockService, so to be compatible again, we should just copy those methods that don't exist in these classes, we cannot change the signature of those methods. IMHO the compatibility with BlockBundle 4 should be done in another PR.

Yet, somehow these blocks work with my changes. they do not need all of those methods.

haivala · 2020-04-10T19:07:49Z

This is really frustrating.

wbloszyk · 2020-04-10T19:16:54Z

@haivala Change this PR to fix only. Focus on:

remove BlockBundle 4 from composer
restore methods from AbstractAdminBlockService
add deprecation notes

In another we can focus on BlockBundle 4 support:

add EditableBlockService

core23 · 2020-05-03T11:23:26Z

Any progress here? Should we revert the change?

wbloszyk · 2020-05-03T11:48:41Z

I think revert 9533247 PR will be the best option now.

This PR will be resolve in #700

fix #1731

a407784

core23 requested changes Apr 6, 2020

View reviewed changes

src/Block/GalleryBlockService.php Outdated Show resolved Hide resolved

haivala added 3 commits April 6, 2020 11:16

remove one comment and fix lint

69de70b

fix lint

db3704e

remove comments

030f041

core23 previously approved these changes Apr 6, 2020

View reviewed changes

core23 added the patch label Apr 6, 2020

core23 requested a review from franmomu April 6, 2020 11:09

phansys previously approved these changes Apr 6, 2020

View reviewed changes

add validateBlock too so block can be created

4be6b0f

haivala dismissed stale reviews from phansys and core23 via 4be6b0f April 6, 2020 14:54

haivala requested review from core23 and phansys April 6, 2020 14:55

core23 previously approved these changes Apr 6, 2020

View reviewed changes

Add EditabBlockService to MediaBlockService

7ed46d7

haivala dismissed core23’s stale review via 7ed46d7 April 8, 2020 05:32

fix lint error

9541daf

haivala changed the title ~~fix #1731~~ WIP: fix #1731 Apr 8, 2020

Add Methods to all blocks

7df47c0

haivala requested a review from core23 April 8, 2020 07:06

haivala added 2 commits April 8, 2020 10:14

fix lint

c4a7145

fix lint

116f48d

haivala changed the title ~~WIP: fix #1731~~ WIP: Add BlockBundle 4.0 support and fix #1731 Apr 8, 2020

phansys requested changes Apr 8, 2020

View reviewed changes

src/Block/GalleryBlockService.php Outdated Show resolved Hide resolved

src/Block/GalleryBlockService.php Show resolved Hide resolved

delete it -> Remove this method

2652129

haivala added 3 commits April 8, 2020 22:57

depr messages and $formMapper->getAdmin() to $this->container->get('s…

bee56aa

…onata.page.admin.block')

CS-fixer

2cdc1d3

cs

8050085

haivala requested a review from phansys April 9, 2020 05:42

haivala added 2 commits April 9, 2020 09:18

dont break 3.0

f4eefb3

remove comma

3ddc959

haivala commented Apr 9, 2020

View reviewed changes

haivala changed the title ~~WIP: Add BlockBundle 4.0 support and fix #1731~~ Add BlockBundle 4.0 support and fix #1731 Apr 9, 2020

wbloszyk requested changes Apr 9, 2020

View reviewed changes

haivala changed the title ~~Add BlockBundle 4.0 support and fix #1731~~ Prepare for BlockBundle 4.0 support and fix #1731 Apr 9, 2020

wbloszyk requested changes Apr 9, 2020

View reviewed changes

wbloszyk mentioned this pull request Apr 24, 2020

Upgrade block-bundle to 4.x in sonata-project bundles sonata-project/SonataBlockBundle#700

Closed

17 tasks

wbloszyk mentioned this pull request May 5, 2020

Fix #1731 #1740

Merged

jordisala1991 closed this Jun 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare for BlockBundle 4.0 support and fix #1731 #1732

Prepare for BlockBundle 4.0 support and fix #1731 #1732

haivala commented Apr 6, 2020 •

edited

Loading

phansys left a comment

haivala commented Apr 6, 2020

haivala commented Apr 6, 2020 •

edited

Loading

phansys commented Apr 7, 2020

wbloszyk commented Apr 7, 2020

phansys commented Apr 7, 2020

haivala commented Apr 8, 2020 •

edited

Loading

haivala commented Apr 8, 2020 •

edited

Loading

haivala commented Apr 8, 2020

haivala commented Apr 8, 2020

haivala commented Apr 8, 2020

phansys commented Apr 8, 2020

haivala commented Apr 8, 2020 •

edited

Loading

haivala Apr 9, 2020

phansys May 6, 2020

haivala commented Apr 9, 2020

wbloszyk Apr 8, 2020

haivala Apr 10, 2020

wbloszyk Apr 9, 2020

wbloszyk Apr 9, 2020

haivala Apr 9, 2020

wbloszyk Apr 9, 2020

haivala Apr 9, 2020

wbloszyk Apr 9, 2020

haivala Apr 9, 2020

franmomu Apr 10, 2020 •

edited

Loading

haivala Apr 10, 2020

haivala commented Apr 10, 2020

wbloszyk commented Apr 10, 2020

core23 commented May 3, 2020

wbloszyk commented May 3, 2020

	use Sonata\BlockBundle\Form\Mapper\FormMapper;
	use Sonata\AdminBundle\Form\Mapper\FormMapper as AdminFormMapper;
	use Sonata\BlockBundle\Form\Mapper\FormMapper;

Prepare for BlockBundle 4.0 support and fix #1731 #1732

Prepare for BlockBundle 4.0 support and fix #1731 #1732

Conversation

haivala commented Apr 6, 2020 • edited Loading

Subject

Changelog

phansys left a comment

Choose a reason for hiding this comment

haivala commented Apr 6, 2020

haivala commented Apr 6, 2020 • edited Loading

phansys commented Apr 7, 2020

wbloszyk commented Apr 7, 2020

phansys commented Apr 7, 2020

haivala commented Apr 8, 2020 • edited Loading

haivala commented Apr 8, 2020 • edited Loading

haivala commented Apr 8, 2020

haivala commented Apr 8, 2020

haivala commented Apr 8, 2020

phansys commented Apr 8, 2020

haivala commented Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haivala commented Apr 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franmomu Apr 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haivala commented Apr 10, 2020

wbloszyk commented Apr 10, 2020

core23 commented May 3, 2020

wbloszyk commented May 3, 2020

haivala commented Apr 6, 2020 •

edited

Loading

haivala commented Apr 6, 2020 •

edited

Loading

haivala commented Apr 8, 2020 •

edited

Loading

haivala commented Apr 8, 2020 •

edited

Loading

haivala commented Apr 8, 2020 •

edited

Loading

franmomu Apr 10, 2020 •

edited

Loading