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.

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

Fix null title/description configuration (#1563 followup) #1577

Merged
merged 1 commit into from
Dec 20, 2017
Merged

Fix null title/description configuration (#1563 followup) #1577

merged 1 commit into from
Dec 20, 2017

Conversation

soyuka
Copy link
Member

@soyuka soyuka commented Dec 20, 2017

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #1563 (comment)
License MIT
Doc PR na

ping @chalasr @dunglas @lyrixx

dunglas
dunglas approved these changes Dec 20, 2017
View reviewed changes
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

🎆

@lyrixx
Copy link
Contributor

lyrixx commented Dec 20, 2017 •
edited
Loading

It's useless IMHO. I adds more code for nothing.
If a user wants an empty title: he does not set the key.
If a he wants a title, he add title: something. Just adding title:~ is useless.

Note: Nobody want an empty title, so raising an exception when using title: is perfectly fine IMHO.

About this comment:

I agree we must allow empty strings or it will be a big BC break (for instance the Flex recipe doesn’t set those keys).

If the key is not present in the YAML, then the value will be de default value and so ''.

@soyuka
Copy link
Member Author

soyuka commented Dec 20, 2017

If the key is not present in the YAML, then the value will be de default value and so ''.

Oh okay we had a doubt on cannotBeEmpty() and defaultValue(''). WDYT @dunglas?

@dunglas
Copy link
Member

dunglas commented Dec 20, 2017

It would be nice to have a test to be sure :)

@soyuka soyuka force-pushed the proper-fix-confi branch 2 times, most recently from 58f081d to 9c97f40 Compare December 20, 2017 10:17
@dunglas dunglas merged commit 15db1f3 into api-platform:2.1 Dec 20, 2017
@dunglas
Copy link
Member

dunglas commented Dec 20, 2017

Thanks @soyuka

@soyuka soyuka deleted the proper-fix-confi branch December 20, 2017 15:36
hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018
@dunglas
Merge pull request api-platform#1577 from soyuka/proper-fix-confi
257d5df 
Fix null title/description configuration (api-platform#1563 followup)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dunglas dunglas dunglas approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@soyuka @lyrixx @dunglas @chalasr