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

Allow replace AbstractAdminBlockService by EditableBlockService #764

Closed

Conversation

wbloszyk
Copy link
Member

@wbloszyk wbloszyk commented Oct 9, 2020

Subject

Based on sonata-project/dev-kit#785 AbstractAdminBlockService should be replace by extends AbstractBlockService implements EditableBlockService. To do it this methods should be replaced:

  • buildEditForm => configureEditForm
  • buildCreateForm => confiureCreateForm
  • validateBlock => validate
  • getBlockMetadata => getMetadata

This change will allow move code to new methods with keep BC. In second step in 4.x branch we can add AbstractAdminBlockService implement EditableBlockService. In third step we can add support for BlockBundle v4 in sonata 3 (with some little BC-break).

I am targeting this branch, because this change respect BC.

Changelog

### Added
- Added `Sonata\BlockBundle\Form\Mapper\BlockFormMapper` class for allow use `FormMapper` with AdminBundle
- Added some methods to `Sonata\BlockBundle\Block\Service\AbstractAdminBlockService` for feature compatibility:
    - `configureEditForm()`
    - `configureCreateForm()`
    - `validate()`
    - `getMetadata()`

@VincentLanglet
Copy link
Member

In third step we can add support for BlockBundle v4 in sonata 3 (with some little BC-break).

We should not.

  • Every BC-break should be forbidden on current branch
  • We should focus on Sonata 4 instead

We just need to support BlockBundle 4 on Sonata 4.

@@ -31,45 +34,97 @@
*/
abstract class AbstractAdminBlockService extends AbstractBlockService implements AdminBlockServiceInterface
Copy link
Member

Choose a reason for hiding this comment

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

This class is already deprecated, no need to change anything right here


namespace Sonata\BlockBundle\Form\Mapper;

use Sonata\AdminBundle\Form\FormMapper as AdminFormMapper;
Copy link
Member

Choose a reason for hiding this comment

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

We should not need any AdminBundle reference. We decoupled this bundle from the admin bundle on the 4.x branch

Copy link
Member Author

Choose a reason for hiding this comment

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

This code is use only when user generate form for Block in Admin site.

Copy link
Member

Choose a reason for hiding this comment

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

It would still need to require sonataAdmin on master https://github.com/sonata-project/SonataBlockBundle/blob/master/composer.json

The right way should be to add this code on sonataAdmin instead, if necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

So it could be directly added to the PageBundle then

Copy link
Member

Choose a reason for hiding this comment

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

So it could be directly added to the PageBundle then

Already done ;)

@github-actions
Copy link

github-actions bot commented Apr 9, 2021

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Apr 9, 2021
@github-actions github-actions bot closed this Apr 16, 2021
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