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

[Serializer] Remove last deprecated/obsolete paths #31771

Merged
merged 1 commit into from Jun 8, 2019

Conversation

Projects
None yet
5 participants
@ogizanagi
Copy link
Member

commented May 31, 2019

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #28316, #28709, #31030, #27020, #29896, 16f8a13#r201060750
License MIT
Doc PR N/A

This should fix the last deprecations & obsolete code paths for the Serializer component.

@ogizanagi ogizanagi added this to the 5.0 milestone May 31, 2019

@ogizanagi ogizanagi requested a review from dunglas as a code owner May 31, 2019

@ogizanagi ogizanagi force-pushed the ogizanagi:rm_deprec/serializer/last branch 2 times, most recently from 38b50d3 to 8dadb20 May 31, 2019

if (class_exists(MimeTypes::class)) {
$mimeTypeGuesser = MimeTypes::getDefault();
} else {
@trigger_error(sprintf('Passing null to "%s()" without symfony/mime installed is deprecated since Symfony 4.3, install symfony/mime.', __METHOD__), E_USER_DEPRECATED);

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 1, 2019

Member

this should be an exception now?

This comment has been minimized.

Copy link
@ogizanagi

ogizanagi Jun 1, 2019

Author Member

That's what I thought too (see second commit), but the data uri normalizer used to fallback to application/octet-stream if no default mime type guesser can be instantiated. So neither was the http-foundation nor the mime type component a strict dependency to use it.

This comment has been minimized.

Copy link
@chalasr

chalasr Jun 1, 2019

Member

this notice is wrong in the first place and should be removed from 4.3 then, right?

This comment has been minimized.

Copy link
@ogizanagi

ogizanagi Jun 7, 2019

Author Member

Indeed. See #31923

@ogizanagi ogizanagi force-pushed the ogizanagi:rm_deprec/serializer/last branch 2 times, most recently from 2482cb3 to fc17011 Jun 1, 2019

if (class_exists(MimeTypes::class)) {
$mimeTypeGuesser = MimeTypes::getDefault();
} else {
@trigger_error(sprintf('Passing null to "%s()" without symfony/mime installed is deprecated since Symfony 4.3, install symfony/mime.', __METHOD__), E_USER_DEPRECATED);

This comment has been minimized.

Copy link
@chalasr

chalasr Jun 1, 2019

Member

this notice is wrong in the first place and should be removed from 4.3 then, right?

Show resolved Hide resolved src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php Outdated

@ogizanagi ogizanagi force-pushed the ogizanagi:rm_deprec/serializer/last branch from fc17011 to 0551695 Jun 7, 2019

* @param object $object
* @param string|null $format
* @param array $context
* @param object $object

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 7, 2019

Member

we can add the type and remove the annotation actually - PHP7.2 allows it

*/
protected function createChildContext(array $parentContext, $attribute/*, ?string $format */)
protected function createChildContext(array $parentContext, string $attribute, ?string $format): array

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 7, 2019

Member

should have null as default, otherwise that's a hard BC break

This comment has been minimized.

Copy link
@ogizanagi

ogizanagi Jun 7, 2019

Author Member

The method is internal. That should be fine, right?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 7, 2019

Member

@internal is not transitive
once all are correctly annotated on 4.4, sure :)

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 8, 2019

Member

fixed in 2b8e441

@ogizanagi ogizanagi force-pushed the ogizanagi:rm_deprec/serializer/last branch from 0551695 to ca1c974 Jun 7, 2019

nicolas-grekas added a commit that referenced this pull request Jun 7, 2019

bug #31923 [Serializer] Fix DataUriNormalizer deprecation (MIME type …
…guesser is optional) (ogizanagi)

This PR was merged into the 4.3 branch.

Discussion
----------

[Serializer] Fix DataUriNormalizer deprecation (MIME type guesser is optional)

| Q             | A
| ------------- | ---
| Branch?       | 4.3 <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | N/A   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | N/A

Relates to #31771 (comment) :

The deprecation isn't fundamentally wrong, but if none of the Mime nor HttpFoundation components are installed, the DataUriNormalizer can be used, and the MIME type used will always be `'application/octet-stream'`

https://github.com/symfony/symfony/blob/9691519ca46511005bc701ca93c2973923952034/src/Symfony/Component/Serializer/Normalizer/DataUriNormalizer.php#L39-L46

https://github.com/symfony/symfony/blob/9691519ca46511005bc701ca93c2973923952034/src/Symfony/Component/Serializer/Normalizer/DataUriNormalizer.php#L135-L139

So this completes the deprecation message, as well as allowing `null` again when no default MIME type guesser is available at all.

Commits
-------

2740bd1 [Serializer] Fix DataUriNormalizer deprecation (MIME type guesser is optional)

@ogizanagi ogizanagi force-pushed the ogizanagi:rm_deprec/serializer/last branch from ca1c974 to c703b35 Jun 7, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

Thank you @ogizanagi.

@nicolas-grekas nicolas-grekas merged commit c703b35 into symfony:master Jun 8, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Jun 8, 2019

minor #31771 [Serializer] Remove last deprecated/obsolete paths (ogiz…
…anagi)

This PR was merged into the 5.0-dev branch.

Discussion
----------

[Serializer] Remove last deprecated/obsolete paths

| Q             | A
| ------------- | ---
| Branch?       | master <!-- see below -->
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #28316, #28709, #31030, #27020, #29896, 16f8a13#r201060750   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | N/A <!-- required for new features -->

This should fix the last deprecations & obsolete code paths for the Serializer component.

Commits
-------

c703b35 [Serializer] Remove last deprecated/obsolete paths

@ogizanagi ogizanagi deleted the ogizanagi:rm_deprec/serializer/last branch Jun 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.