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

[Review] Added detailed Backwards Compatibility Promise text #3439

Merged
merged 39 commits into from
Feb 20, 2014

Conversation

webmozart
Copy link
Contributor

I created a detailed text about which parts of the Symfony code are covered by our BC promise. The goals are to:

  • tell our users which parts exactly are safe to use
  • tell our contributors which changes they are allowed to do

I'll continue working on the individual rules of allowed changes. Nevertheless, let me hear your feedback.

Update (2014/01/10)

The draft is ready for review now.

Our Backwards Compatibility Promise
===================================

As of Symfony 2.1, we promise you backwards compatibility for all further 2.x
Copy link
Member

Choose a reason for hiding this comment

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

I would say 2.3 here as this is more accurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Member

Choose a reason for hiding this comment

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

then this should also be merged into 2.3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will open another PR when everything else is finished.

Change name No No
Reduce visibility Yes [1]_ No
Add parameter without a default value Yes [1]_ No
Add parameter with a default value Yes Yes
Copy link
Contributor

Choose a reason for hiding this comment

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

But "Add parameter with a default value" still breaks code if someone has overwritten the method in their class. Maybe also adding [1] here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, thank you!

However, backwards compatibility comes in many different flavors. This page
lists which code changes are covered by our promise and to what degree.

If you are contributing to Symfony, make sure that your code changes comply to
Copy link
Contributor

Choose a reason for hiding this comment

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

I would perhaps say "....comply with the rules" rather than "comply to". Either that or "adhere to".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! Language suggestions are more than welcome.

however be documented in the UPGRADE file.

.. [2] Should be avoided. When done, this change must be documented in the
UGPRADE file.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: UPGRADE (apologies, I added this comment previously on an outdated diff)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

fabpot added a commit to symfony/symfony that referenced this pull request Jan 8, 2014
This PR was merged into the 2.5-dev branch.

Discussion
----------

Upgrade File for 2.5

Added upgrade info for #9601, as this pr might break code which overwrites this method and also to respect symfony/symfony-docs#3439.

Commits
-------

fefcf41 Added upgrade info for #9601
@fabpot
Copy link
Member

fabpot commented Feb 20, 2014

@webmozart The document looks very good to me; I've made some comments about minor changes.

Thank you so much for taking time to thoroughly document what BC means for us. I'm very happy with the result and it will make discussion about future changes so much easier.

@webmozart
Copy link
Contributor Author

Thanks @fabpot for the comments! I will adapt the document later today.

@webmozart
Copy link
Contributor Author

@fabpot I adapted the document now. Any more feedback?

Ensuring smooth upgrades of your projects is our first priority. That's why
we promise you backwards compatibility (BC) for all minor Symfony releases.
You probably recognize this strategy as `Semantic Versioning`_. In short,
Semantic Versioning means that only major releases (such as 2.0, 3.0 etc.) are
Copy link
Contributor

Choose a reason for hiding this comment

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

3.0, etc.

@fabpot
Copy link
Member

fabpot commented Feb 20, 2014

@webmozart Looks good to me

weaverryan added a commit that referenced this pull request Feb 20, 2014
… text (webmozart)

This PR was merged into the 2.1 branch.

Discussion
----------

[Review] Added detailed Backwards Compatibility Promise text

I created a detailed text about which parts of the Symfony code are covered by our BC promise. The goals are to:

* tell our users which parts exactly are safe to use
* tell our contributors which changes they are allowed to do

I'll continue working on the individual rules of allowed changes. Nevertheless, let me hear your feedback.

##### Update (2014/01/10)

The draft is ready for review now.

Commits
-------

0717192 Removed useless line break
ce58ee9 Added rules for adding parent interfaces and moving methods/properties to parent interfaces/classes
be2251c Implemented @fabpot's comments
90c4de6 Mentioned Semantic Versioning in the introduction
2320906 Extracted duplicated text into _api_tagging.rst.inc
bdd3c03 Implemented changes suggested by @wouterj
fd1d912 Typo
11bb879 Grammar
25443c0 Improved table formatting
e11335f Improved the wording of the "Using Symfony" section
4868452 Added prose about the difference between regular/API classes/interfaces
8c6c7bf Simplified usage description
6d9edf1 Improved wording: Changed "safe" to "guaranteed"
ef1f021 Added note about test classes
5a160c5 Added note about deprecated interfaces/classes
69768dd Improved wording: use -> call, access
0c6420f Added information about changing parameter types
6501a35 Added information about changing return types that are classes or interfaces
dfb3e8b Improved wording
be76644 Added information about internal classes and interfaces
af3a645 Added note about requesting `@api` tags
dcbe79a Improved wording
efd3911 Added that adding custom properties is not safe
00c6ebe Fixed safety statements
54fd836 Language improvements
db76288 Fixed headings
c6e850d Language fixes
4c5a55d Rearranged page to have different sections for different user bases
502ed95 Added: Some breaking changes to unsafe operations are documented in the UPGRADE file
31ab2db Improved wording
a3ad08c Removed most of the "cannot" statements which are repeated in the tables now
345410c Rearranged safe operations to make more sense
afadaab Changed: The last parameters of a method may be removed
44ecf16 Fixed: No parameters must be added (ever) to API class methods
0e925cb Added tables with safe operations
79ca9f7 Added information about type compatibility
dacd7ce Rearranged rules to be more easily understandable
7320ed0 Updated BC promise to be valid as of Symfony 2.3
840073c Added detailed BC promise text
@weaverryan weaverryan merged commit 0717192 into symfony:2.1 Feb 20, 2014
@weaverryan
Copy link
Member

Nice work @webmozart - it reads very very well. And nice use of footnotes - I hadn't seen those before.

Cheers!

@webmozart
Copy link
Contributor Author

Thanks for merging! :)

Who's in charge of doing the styling of the docs? The footnotes could use some love :) Also, it would be nice if the links would be rendered as superscripted numbers, like in books.

I just realized we need to discuss one more thing, namely the changing of method return values in regular classes/interfaces (but not in API classes/interfaces). The current version allows (among others) to change the return values of Symfony methods from:

Original Type To New Type
array instance of ArrayAccess, Traversable and Countable
ArrayAccess array
Traversable array
Countable array

Contrary to the other changes listed in the table, these changes might break applications:

Examples (rule 1):
$array = $object->getFoo();

// fine if changed to an object
$array[1] = 'bar';
foreach ($array as $key => $value) { ... }
echo count($array);

// breaks if changed to an object
array_walk($array, 'callback');

// easy fix
$array = iterator_to_array($object->getFoo());
array_walk($array, 'callback');
Examples (rule 2):
$array = $object->getFoo();

// fine if changed to an array
$array[1] = 'bar';
echo $array[0];

// breaks if changed to an array
$array->offsetSet(1, 'baz');

// easy fix
$array = new \ArrayObject($object->getFoo());
$array->offsetSet(1, 'baz');

Rule 3 and 4 are basically identical to rule 2.

I originally allowed these changes because the upgrade path is so simple (iterator_to_array()/new \ArrayObject()), however do you agree with this?

A specific use case for these rules is symfony/symfony#9918.

@webmozart webmozart deleted the bc-promise branch February 20, 2014 21:04
@wouterj
Copy link
Member

wouterj commented Feb 20, 2014

Who's in charge of doing the styling of the docs? The footnotes could use some love :) Also, it would be nice if the links would be rendered as superscripted numbers, like in books.

ping @javiereguiluz

@javiereguiluz
Copy link
Member

@wouterj @webmozart I can take care of that. I'll return with more information when I know exactly when the new styles can be applied on the symfony.com website.

@webmozart
Copy link
Contributor Author

Thanks @javiereguiluz! You're the best! :)

@javiereguiluz
Copy link
Member

@wouterj @webmozart here it is the before/after comparison of the style of the footnotes. Please, tell me anything else that you'd like to change in this proposal and we'll see it in production soon:

footnotes

table_footnotes

@wouterj
Copy link
Member

wouterj commented Feb 21, 2014

Thanks @javiereguiluz for involving us in the process! :)

I like it, while I think the numbers on the footnote (first image, (1,2,3,4...)) should be somewhat smaller too, they are not that important imo

@javiereguiluz
Copy link
Member

@wouterj maybe we should hide those numbers? I don't know if they are useful at all. If you click on a footnote, there is no way to know which number you should go back.

@wouterj
Copy link
Member

wouterj commented Feb 21, 2014

@javiereguiluz yes, that's an option too. But please note that if the footnote only applies to one occurence, the footnote number will be a link instead. I think it's harder to remove that one?

@javiereguiluz
Copy link
Member

In the case of unique footnotes, we can disguise the footnote link simply by changing the font color.

@wouterj
Copy link
Member

wouterj commented Feb 21, 2014

Ok, let's do that :) (and also add cursor:default; to remove the pointer cursor ;) )

@webmozart
Copy link
Contributor Author

@javiereguiluz Awesome! Yes, I would like to hide the backlinks too ( (1,2,3,4,..) ) if that's possible with the RST parser. Is it possible to remove the squared brackets around the numbers?

@webmozart
Copy link
Contributor Author

Also, could you change the table style so that the grey horizontal lines don't cut through the black border of the table? Should be possible by moving the black border from the cells to the table itself.

@javiereguiluz
Copy link
Member

  • The backlink numbers are now hidden.
  • I've asked if we can remove the square brackets around footnote numbers, but I'm not very confident about being possible.
  • Definitely the styles of the tables need to be fixed. I haven't done it yet because we are postponing those style changes for the upcoming symfony.com redesign (we will announce some things about this soon).

@webmozart
Copy link
Contributor Author

Cool, thanks @javiereguiluz! I just realized that the fonts in the footnotes are very different. One is the font of the table content ([1]-[4]), the other is the one of the paragraphs in the normal page content ([5]-[6]). Is this waiting for the redesign too?

@javiereguiluz
Copy link
Member

@webmozart you are right! Font normalization is going to be one of the biggest tasks for the redesign.

@wouterj
Copy link
Member

wouterj commented Apr 14, 2014

@javiereguiluz you forgot to remove the link style for hover. You'll get a pointer cursor and an underline when hovering over the numbers in the footnotes.

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

Successfully merging this pull request may close these issues.

9 participants