Skip to content

[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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 10, 2017
Merged

[Config] Fix cannotBeEmpty() #24633

merged 1 commit into from
Nov 10, 2017

Conversation

ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Oct 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

@linaori
Copy link
Contributor

linaori commented Oct 19, 2017

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

@ro0NL
Copy link
Contributor Author

ro0NL commented Oct 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 nicolas-grekas added this to the 3.4 milestone Oct 19, 2017
@linaori
Copy link
Contributor

linaori commented Oct 19, 2017

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

@ro0NL
Copy link
Contributor Author

ro0NL commented Oct 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

@linaori
Copy link
Contributor

linaori commented Oct 25, 2017

Yes, that message sounds good! 👍

@ogizanagi
Copy link
Contributor

ping @symfony/deciders

@fabpot
Copy link
Member

fabpot commented Nov 10, 2017

Thank you @ro0NL.

@fabpot fabpot merged commit 2269f70 into symfony:3.4 Nov 10, 2017
fabpot added a commit that referenced this pull request Nov 10, 2017
@fabpot
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 @iltar

Commits
-------

2269f70 [Config] Fix cannotBeEmpty()
@linaori
Copy link
Contributor

linaori commented Nov 10, 2017

Awesome, thanks for fixing it! ❤️

@ro0NL ro0NL deleted the config/cannot-be-empty branch November 10, 2017 15:55
This was referenced Nov 12, 2017
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@stof stof stof approved these changes

@xabbuh xabbuh xabbuh approved these changes

@ogizanagi ogizanagi ogizanagi approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Bug Config Deprecation Status: Reviewed
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
9 participants
@ro0NL @linaori @ogizanagi @fabpot @stof @xabbuh @chalasr @nicolas-grekas @carsonbot