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

[Contributing][Code] do not distinguish regular classes and API classes #5735

Merged
merged 1 commit into from
Nov 5, 2015

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Sep 28, 2015

Q A
Doc fix? no
New docs? kind of (symfony/symfony#15977, symfony/symfony#15979)
Applies to all
Fixed tickets

@stof
Copy link
Member

stof commented Sep 29, 2015

I would just add a small note saying that these BC rules don't apply on classes/methods tagged as @internal

@xabbuh
Copy link
Member Author

xabbuh commented Sep 29, 2015

@stof We already have this caution blocks (and a similar one for classes):

The exception to this rule are interfaces tagged with @internal. Such interfaces should not be used or implemented.

@stof
Copy link
Member

stof commented Sep 29, 2015

Then fine.

@wouterj
Copy link
Member

wouterj commented Oct 5, 2015

👍

| Add a default value to an argument | Yes | Yes |
+-----------------------------------------------+---------------+---------------+
+-----------------------------------------------+-----------------------------+
| Use Case | Backwards Compatibility |
Copy link
Member

Choose a reason for hiding this comment

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

We should rethink this table. In the past some columns were Yes and others No, so the table made sense. But now everything is Yes.

Copy link
Member

Choose a reason for hiding this comment

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

I think a table structure is very nice here. It's clear and it's consistent with the other "promises tables" below.

@weaverryan
Copy link
Member

Nice work Christian - thanks!

@weaverryan weaverryan merged commit 9794d58 into symfony:2.3 Nov 5, 2015
weaverryan added a commit that referenced this pull request Nov 5, 2015
… and API classes (xabbuh)

This PR was merged into the 2.3 branch.

Discussion
----------

[Contributing][Code] do not distinguish regular classes and API classes

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | kind of (symfony/symfony#15977, symfony/symfony#15979)
| Applies to    | all
| Fixed tickets |

Commits
-------

9794d58 do not distinguish regular classes and API classes
@xabbuh xabbuh deleted the symfony-15977 branch November 5, 2015 16:56
Reduce visibility No
Move to parent class Yes
Add argument without a default value No
Add argument with a default value No
Copy link
Contributor

Choose a reason for hiding this comment

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

This was a brave change. Now it's impossible to extend existing public code with new functionality. Was this intended?

see symfony/symfony#16834

Copy link
Member Author

Choose a reason for hiding this comment

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

This was the only option we had as PHP will complain anyway (see https://3v4l.org/6T2DO).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants