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

Update MediaManager.php #1871

Merged
merged 1 commit into from
Jun 6, 2021
Merged

Conversation

mrcmorales
Copy link
Contributor

@mrcmorales mrcmorales commented Nov 26, 2020

Subject

I am targeting this branch, because {reason}.

Closes #1870.

Changelog

### Fixed
- `MediaManager` implements `MediaManagerInterface`

@wbloszyk wbloszyk requested a review from a team February 16, 2021 20:37
Copy link
Member

@phansys phansys left a comment

Choose a reason for hiding this comment

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

Good job @mrcmorales!
Could you please add a test in order to avoid regressions?

@VincentLanglet
Copy link
Member

Good job @mrcmorales!
Could you please add a test in order to avoid regressions?

Can we still merge this @phansys ? It would be nice to avoid blocking a bugfix like this.

@core23
Copy link
Member

core23 commented Jun 6, 2021

Could you please add a test in order to avoid regressions?

Do we need tests that a class contains some interface? oO

Copy link
Member

@phansys phansys left a comment

Choose a reason for hiding this comment

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

Can we still merge this @phansys ? It would be nice to avoid blocking a bugfix like this.

Sure, we could add some tests later.

Do we need tests that a class contains some interface? oO

If this is considered as bugfix, I think so. A call to assertInstanceOf() should be enough IMO.

@VincentLanglet VincentLanglet merged commit 78d46a8 into sonata-project:3.x Jun 6, 2021
@core23
Copy link
Member

core23 commented Jun 6, 2021

If this is considered as bugfix, I think so. A call to assertInstanceOf() should be enough IMO.

A test should cover behavior not the design of a class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document\MediaManager dosen't implements Model\MediaManagerInterface.
5 participants