Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] add a notice when passing a routerInterface without warmupInterface in RouterCacheWarmer#24894
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
f437cf8 to66d49f3Compare| if ($this->routerinstanceof WarmableInterface) { | ||
| $this->router->warmUp($cacheDir); | ||
| }else { | ||
| trigger_error(sprintf('%s is not a instanceof %s, no warmUp has been triggered.',get_class($this->router), WarmableInterface::class), \E_USER_NOTICE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMHO we could trigger a deprecation now and throw a LogicException at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ddo we ever trigger a notice in the code base? This is suspicious to me :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We trigger warning but not notices you are right, I will change that with@chalasr's suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Since we are triggering a deprecation, we should go with master ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, triggering/throwing does not fix a bug, it only enhances DX. This is for master in any case IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's for 4.1 IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Rebase and throw added.
66d49f3 to6656dbeCompare0458373 to6df98e5Compare| return; | ||
| }else { | ||
| @trigger_error(sprintf('Passing a %s without implementing %s is deprecated since 4.1.', RouterInterface::class, WarmableInterface::class), \E_USER_DEPRECATED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I do not understand why we care about a deprecation if we immediately throw afterwards anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We MUST NOT throw (we cannot do it until 5.0). We should have only the deprecation notice.
| return; | ||
| }else { | ||
| @trigger_error(sprintf('Passing a %s without implementing %s is deprecated since 4.1.', RouterInterface::class, WarmableInterface::class), \E_USER_DEPRECATED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We MUST NOT throw (we cannot do it until 5.0). We should have only the deprecation notice.
6df98e5 to4128cf8CompareSimperfit commentedNov 19, 2017
done@stof |
4128cf8 tocfd840fCompareSimperfit commentedNov 19, 2017
I will update the Changelog |
Simperfit commentedNov 19, 2017
Travis failure seems unrelated |
| $router->warmUp($cacheDir); | ||
| return; | ||
| }else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
no need for theelse as the previousif returns anyway
cfd840f to060f65aCompareSimperfit commentedDec 5, 2017
Simperfit commentedDec 5, 2017
Status: Needs Review |
6e94f29 to18453aaCompareUPGRADE-4.1.md Outdated
| * The`FileDumper::setBackup()` method is deprecated and will be removed in 5.0. | ||
| * The`TranslationWriter::disableBackup()` method is deprecated and will be removed in 5.0. | ||
| FrameworkBundle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry for nitpicking@Simperfit, but could you please move this up sort entries alphabetically (makes searching for something much easier in the future with many more entries)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
18453aa to06faa74Comparechalasr commentedDec 12, 2017
Needs rebase |
| return; | ||
| } | ||
| @trigger_error(sprintf('Passing a %s without implementing %s is deprecated since 4.1.', RouterInterface::class, WarmableInterface::class), \E_USER_DEPRECATED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
since Symfony 4.1
06faa74 to879a720CompareSimperfit commentedJan 5, 2018
PR rebased |
879a720 tob5d9048Compare…rmupInterface in RouterCacheWarmer
b5d9048 todaa7f02CompareSimperfit commentedJan 24, 2018
PR Rebased |
Simperfit commentedJan 26, 2018
@nicolas-grekas this one is ready. |
fabpot commentedJan 26, 2018
Thank you@Simperfit. |
…terface without warmupInterface in RouterCacheWarmer (Simperfit)This PR was merged into the 4.1-dev branch.Discussion----------[FrameworkBundle] add a notice when passing a routerInterface without warmupInterface in RouterCacheWarmer| Q | A| ------------- | ---| Branch? | master| Bug fix? | yesish| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets |#23403| License | MIT| Doc PR | noneI'm adding a test to RouterCacheWarmer since there were none.Commits-------daa7f02 [FrameworkBundle] add a notice when passing a routerInterface with warmupInterface in RouterCacheWarmer
| FrameworkBundle | ||
| --------------- | ||
| * Using a`RouterInterface` that does not implement the`WarmableInterface` is not supported anymore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This must mention that it is about RouterCacheWarmer. This has nothing to do with the RouterInterface itself.
…ions (xabbuh)This PR was merged into the 4.1-dev branch.Discussion----------[FrameworkBundle] clarify changelog and upgrade instructions| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |symfony/symfony#24894 (comment)| License | MIT| Doc PR |Commits-------a8df0aee92 clarify changelog and upgrade instructions
…ions (xabbuh)This PR was merged into the 4.1-dev branch.Discussion----------[FrameworkBundle] clarify changelog and upgrade instructions| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#24894 (comment)| License | MIT| Doc PR |Commits-------a8df0ae clarify changelog and upgrade instructions
This PR was merged into the 5.0-dev branch.Discussion----------[Routing] remove deprecations| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| BC breaks? | yes <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets || License | MIT| Doc PR |Ref.#30249,#30286,#24894Commits-------5beb5f8 [Routing] remove deprecations
Uh oh!
There was an error while loading.Please reload this page.
I'm adding a test to RouterCacheWarmer since there were none.