added upgrade note for deprecated container property #1089
Conversation
Makes me hungry ;) |
|
We don't need this, because the property is already marked as |
Is this enough? We are providing deprecation messages, but for each of them, we also have an upgrade note, right? |
|
IMO, we have enough information for developers. It is marked as |
|
Was requested by @soullivaneuh FYI And it is also written here under the Deprecated title : https://github.com/sonata-project/SonataMediaBundle/releases/tag/3.1.0 |
|
Let's reopen and assign this to @soullivaneuh , ok? |
|
@soullivaneuh should we add this note to the |
It was always like this. All deprecation must be documented. BTW, this should not be just be a line with which is deprecated, but a full description with example of before/after implements. Because if we just write a line, indeed the Just see the Symfony upgrades guide for inspiration, it's quite well formatted. |
|
|
||
| ### Deprecated | ||
|
|
||
| `$container` property in `Security/SessionDownloadStrategy` is deprecated. Use `SessionInterface` `$session` instead. |
There was a problem hiding this comment.
Please add example of before/after implementation. Looks symfony upgrade guides for inspirations.
There was a problem hiding this comment.
is it ok like this @soullivaneuh ?
There was a problem hiding this comment.
Looks ok but I'm not an expert of media-bundle so I don't know if the example is enough.
cc @sonata-project/contributors for review.
codebach
force-pushed
the
code-improve-3.x
branch
from
6b98071
to
91bb8f1
Compare
| Before: | ||
|
|
||
| ```php | ||
| $downloadStrategy = new SessionDownloadStrategy($translator, $container, $times); |
There was a problem hiding this comment.
hmm, I am missing the typehint here, maybe its better so show the __construct() method with their typehints here to see the difference? @greg0ire
There was a problem hiding this comment.
I think people know what $container and $session probably refer to ;)
There was a problem hiding this comment.
ok, then lets merge as it is! 👍
|
Thank you @ahmetakbn 👍 |
I am targetting this branch, because it is upgrade not of this branch.
refs #1035