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

Make FeatureMediaBlockService not extend from MediaBlockService #2137

Merged

Conversation

jordisala1991
Copy link
Member

In the process, getStylesheet is removed since it is not used on
BlockBundle 4.0, and load is refactor to ensure we can load the
correct entity with its given id (not necessarily a integer).

Subject

I am targeting this branch, because this breaks BC.

Changelog

### Changed
- Changed `FeatureMediaBlockService` to not extend `MediaBlockService`.
- Changed `MediaBlockService` to be final.

@jordisala1991 jordisala1991 force-pushed the hotfix/remove-unused-function branch from 8f0b7e3 to c44e1c0 Compare September 27, 2021 06:43
In the process, getStylesheet is removed since it is not used on
BlockBundle 4.0, and load is refactor to ensure we can load the
correct entity with its given id (not necessarily a integer).
@jordisala1991 jordisala1991 force-pushed the hotfix/remove-unused-function branch from c44e1c0 to 76cf35b Compare September 27, 2021 16:02
@jordisala1991 jordisala1991 requested review from VincentLanglet and a team September 27, 2021 16:04
@jordisala1991 jordisala1991 force-pushed the hotfix/remove-unused-function branch from b1a98ce to b34bc39 Compare September 28, 2021 07:20
@jordisala1991 jordisala1991 requested review from core23 and a team September 28, 2021 07:23
parent::__construct($twig);

$this->pool = $pool;
$this->mediaAdmin = $mediaAdmin;
Copy link
Member

Choose a reason for hiding this comment

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

This property make some issue.
When we set db_driver: 'no_driver' then sonata.media.admin.media does not exists and throw exception.
We should:
Allow create admin service with manager_type: none (or manager_type: no_driver).
Or make mediaAdmin optional - then also we should not allow generate edit form without mediaAdmin.
Also if we do not have admin, then we should throw exception while generate edit form (is used by EditableBlockService). So can you change it to optional?

Copy link
Member

Choose a reason for hiding this comment

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

Or we should find way to replace them.

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 is working exactly the same as the MediaBlockService.
The no_driver option is not suposed to be used by the users, it is just a temporary option.

If you want to improve that part. Please create another issue explaining why is that important and/or open a Pull request showing some code to implement that part.

@jordisala1991 jordisala1991 requested a review from a team September 28, 2021 07:45
@wbloszyk wbloszyk requested a review from a team September 28, 2021 07:47
@jordisala1991 jordisala1991 merged commit eae8689 into sonata-project:4.x Sep 28, 2021
@jordisala1991 jordisala1991 deleted the hotfix/remove-unused-function branch September 28, 2021 09:10
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