NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Config] Fix cannotBeEmpty()#24633

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:3.4fromro0NL:config/cannot-be-empty

Nov 10, 2017

Merged

[Config] Fix cannotBeEmpty()#24633

fabpot merged 1 commit intosymfony:3.4fromro0NL:config/cannot-be-empty

Nov 10, 2017

Conversation

Copy link

Contributor

ro0NL commentedOct 19, 2017

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#24614
License	MIT
Doc PR	symfony/symfony-docs#...

Open for better deprecation message, but this should clarify.

cc@iltar

carsonbot added Status: Needs Review Config Bug Deprecation labels

Oct 19, 2017

Copy link

Contributor

linaori commentedOct 19, 2017

Small side note, the case where mine triggered, wasn't working withrequiresAtLeastOneElement(), I've already tried that. I'll check tomorrow what it was used on (ping me in case I forget).

Copy link

ContributorAuthor

ro0NL commentedOct 19, 2017•
edited
Loading

@iltar from your issue;

->requiresAtLeastOneElement() is not applicable to concrete nodes

Im not actually sure this relates to the PR of mine 🤔 it should be->cannotBeEmpty is not.. then. So curious about the breakage you experiencing :)

Still 👍 for this path.

nicolas-grekas added this to the3.4 milestone

Oct 19, 2017

Copy link

Contributor

@ro0NL No, that error is not related to the original issue, but your deprecation is sayinguse requiresAtLeastOneElement() instead, which will also error for me, so it might not be the right description

Copy link

ContributorAuthor

ro0NL commentedOct 25, 2017

Updated deprecation message.@iltar OK for you?

I still find your issue confusing; the title refers to cannotBeEmpty(), yet the body mentions requiresAtLeastOneElement. Im assuming cannotBeEmpty() indeed raises this issue in 3.4

Copy link

Contributor

linaori commentedOct 25, 2017

Yes, that message sounds good! 👍

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Nov 10, 2017

ogizanagi approved these changes

Nov 10, 2017

View reviewed changes

[Config] Fix cannotBeEmpty()

2269f70

Copy link

Contributor

ogizanagi commentedNov 10, 2017

ping @symfony/deciders

stof approved these changes

Nov 10, 2017

View reviewed changes

chalasr approved these changes

Nov 10, 2017

View reviewed changes

xabbuh approved these changes

Nov 10, 2017

View reviewed changes

fabpot approved these changes

Nov 10, 2017

View reviewed changes

Copy link

Member

fabpot commentedNov 10, 2017

Thank you@ro0NL.

fabpot merged commit2269f70 intosymfony:3.4

Nov 10, 2017

fabpot added a commit that referenced this pull request

Nov 10, 2017

bug#24633[Config] Fix cannotBeEmpty() (ro0NL)

b7928c3

This PR was merged into the 3.4 branch.Discussion----------[Config] Fix cannotBeEmpty()| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets |#24614| License       | MIT| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->Open for better deprecation message, but this should clarify.cc@iltarCommits-------2269f70 [Config] Fix cannotBeEmpty()

Copy link

Contributor