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 MediaDataProvider if no security is configured #5580

Conversation

danrot
Copy link
Contributor

@danrot danrot commented Oct 30, 2020

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets ---
Related issues/PRs #5439
License MIT
Documentation PR ---

What's in this PR?

This PR fixes the signature of the MediaDataProvider constructor to also accept null for the Security class.

Why?

Because if no security is configured in config/packages/security_website.yaml the Security class is not available and therefore null. In that case the website currently breaks, if a SmartContent displaying media is on the page.

@danrot danrot changed the base branch from master to release/2.2 October 30, 2020 07:52
@danrot danrot force-pushed the bugfix/media-smart-content-without-security branch from d26a7b4 to f5a4b3f Compare October 30, 2020 07:52
Copy link
Member

@alexander-schranz alexander-schranz left a comment

Choose a reason for hiding this comment

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

Optional parameters should be at the end, else it will fail in the next version of php. So maybe we just make it nullable using ?Security $security,

@@ -45,7 +45,7 @@ public function __construct(
ArraySerializerInterface $serializer,
RequestStack $requestStack,
ReferenceStoreInterface $referenceStore,
Security $security,
Security $security = null,
Copy link
Member

Choose a reason for hiding this comment

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

optional parameters in the middle is not longer allowed: https://php.watch/versions/8.0/deprecate-required-param-after-optional

So we would need to add it to the end or use ?Security $security

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woah, that's a heavy change. Gonna fix it.

@danrot danrot force-pushed the bugfix/media-smart-content-without-security branch from ba70d3e to 684a676 Compare October 30, 2020 13:23
@alexander-schranz alexander-schranz added the Bug Error or unexpected behavior of already existing functionality label Nov 2, 2020
@alexander-schranz alexander-schranz merged commit 45abbce into sulu:release/2.2 Nov 2, 2020
@danrot danrot deleted the bugfix/media-smart-content-without-security branch November 2, 2020 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Error or unexpected behavior of already existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants