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 symfony/config deprecation #1153

Merged
merged 8 commits into from Apr 3, 2020

Conversation

jrushlow
Copy link
Contributor

@jrushlow jrushlow commented Apr 1, 2020

Starting in Symfony/Config 5.1, calling NodeDefinition::setDeprecated() with no arguments is deprecated. See symfony/symfony#35871

This addresses the deprecation by providing the soon to be required parameters.

DependencyInjection/Configuration.php Outdated Show resolved Hide resolved
@alcaeus alcaeus added this to the 2.1.0 milestone Apr 1, 2020
@alcaeus alcaeus added this to 2.1 in Roadmap Apr 1, 2020
Co-Authored-By: Andreas Braun <alcaeus@users.noreply.github.com>
@alcaeus
Copy link
Member

alcaeus commented Apr 1, 2020

Changes look good, just waiting to see if travis-ci likes this version better than the previous one. Breakage is most likely unrelated, but I'd like to investigate the failure if it occurs. I'll be merging this once the build is stable. Thanks @jrushlow!

@alcaeus alcaeus self-assigned this Apr 1, 2020
@alcaeus alcaeus closed this Apr 1, 2020
@alcaeus alcaeus reopened this Apr 1, 2020
@@ -91,7 +91,11 @@ private function addDbalSection(ArrayNodeDefinition $node) : void
->end()
->children()
->scalarNode('class')->isRequired()->end()
->booleanNode('commented')->setDeprecated()->end()
->booleanNode('commented')->setDeprecated(
'doctrine/doctrine-bundle',
Copy link
Member

Choose a reason for hiding this comment

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

With symfony/config < 5.1 this change would now mean that the message displayed to the user would be doctrine/doctrine-bundle as the only argument of setDeprecated() used to be the message.

Copy link
Contributor Author

@jrushlow jrushlow Apr 1, 2020

Choose a reason for hiding this comment

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

hmm, would something along the lines of

$messageArray = $this-getMessage();
private func getMessage(): array
{
 if config < 5.1 $message = [] else $message ['doctrine/bundle', 2.0, 'this is why'];
}

....->setDeprecated(...$messageArray)->end()

do the trick?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i believe this is now taken care of. See my comment below...

@@ -91,7 +92,7 @@ private function addDbalSection(ArrayNodeDefinition $node) : void
->end()
->children()
->scalarNode('class')->isRequired()->end()
->booleanNode('commented')->setDeprecated()->end()
->booleanNode('commented')->setDeprecated(...$this->getCommentedParamDeprecationMsg())->end()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if return value of $this->getCommentedParamDeprecationMsg() is [], I believe using ... will cause ArgumentCountError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed - for config versions before 5.1, a message is provided. Attempted to return null for < 5.1 but that triggered a TypeError in an even earlier version before setDeprecated() allowed ?string

@alcaeus alcaeus requested a review from xabbuh April 2, 2020 06:01
@alcaeus
Copy link
Member

alcaeus commented Apr 3, 2020

@jrushlow thanks for the contribution! Merging this as the coverage build is also failing in master. travis-ci not reporting back its result is an issue they've been having that we're trying to work around separately.

@alcaeus alcaeus merged commit bda5f51 into doctrine:master Apr 3, 2020
@jrushlow jrushlow deleted the fix/config-deprecation branch April 3, 2020 15:25
greg0ire pushed a commit to nicolas-grekas/DoctrineBundle that referenced this pull request Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants