8000 BC promise doesn't care about constants · Issue #5224 · symfony/symfony-docs · GitHub
[go: up one dir, main page]
Skip to content

BC promise doesn't care about constants #5224

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
Closed
dosten opened this issue May 3, 2015 · 12 comments
Closed

BC promise doesn't care about constants #5224

dosten opened this issue May 3, 2015 · 12 comments
Labels
actionable Clear and specific issues ready for anyone to take them.

Comments

@dosten
Copy link
Contributor
dosten commented May 3, 2015

Searching in the BC promise i don't find any information about the removal, etc of a constant in a normal or API class. Ex: In symfony/symfony#14531 the PATTERN constant it's not necessarry but i don't know if i can remove it since the class is tagged as @api.
@stof
Copy link
Member
stof commented May 4, 2015

Constants are public in PHP, so they cannot be removed

@dosten
Copy link
Contributor Author
dosten commented May 4, 2015

Agree, what do you think about adding this into the "Changing Interfaces/Classes" tables in the BC promise page?

@stof
Copy link
Member
stof commented May 4, 2015

@dosten sounds good to me. Can you send a PR to improve it ?

@wouterj
Copy link
Member
wouterj commented May 4, 2015

👍

@wouterj wouterj added good first issue Ideal for your first contribution! (some Symfony experience may be required) actionable Clear and specific issues ready for anyone to take them. labels May 4, 2015
@dosten
Copy link
Contributor Author
dosten commented May 4, 2015

Yes, of course

@dosten
Copy link
Contributor Author
dosten commented May 4, 2015

Before sending a PR i want to be sure about the changes allowed.

Type of Change Regular API
Add constant yes no
Remove constant no no
Change value no no

Adding a constant to a API class/interface must be allowed?

@wouterj
Copy link
Member
wouterj commented May 4, 2015

@dosten I would say:

Type of Change Regular API
Add constant yes yes
Remove constant no no
Change value yes no

/cc @fabpot @webmozart

@dosten
Copy link
Contributor Author
dosten commented May 5, 2015

@wouterj I'm not sure about allow to change the value of a constant in a normal class/interface.

@dosten
Copy link
Contributor Author
dosten commented May 7, 2015

ping @fabpot @webmozart

@stof
Copy link
Member
stof commented May 9, 2015

Changing the value of the constant depends of what the goal of the constant is. In most cases, it is not allowed. But we have a constant in the Yaml component providing a regex IIRC, and changing the value to fix a bug in the regex would be allowed for instance

@dosten
Copy link
Contributor Author
dosten commented May 11, 2015

So, we can allow to change the value of a constant in a regular or API class/interface with a footnote?

@wouterj wouterj removed the good first issue Ideal for your first contribution! (some Symfony experience may be required) label Sep 3, 2015
@wouterj wouterj mentioned this issue Sep 3, 2015
Closed
weaverryan added a commit that referenced this issue Jul 10, 2016
@weaverryan
feature #5672 Add constants to BC promise (WouterJ)
c71c312 
This PR was submitted for the 2.8 branch but it was merged into the 2.7 branch instead (closes #5672).

Discussion
----------

Add constants to BC promise

| Q | A
| --- | ---
| Doc fix? | no
| New docs? | yes
| Applies to | all
| Fixed tickets | #5224

This does things a little bit different than proposed in #5224, based on [this comment by @fabpot](symfony/symfony#15680 (comment)) and the fact that one uses constants, to allow to change the value without breaking the code.

/cc @webmozart @stof @weaverryan @xabbuh please review this & give your opinion on this.

Commits
-------

d25c28d Add constants to BC promise
@javiereguiluz
Copy link
Member

Closing it as fixed by #5672.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
actionable Clear and specific issues ready for anyone to take them.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@javiereguiluz @stof @dosten @wouterj
0