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

Fix bc compatiblity between ErrorRenderer FlattenException and Debug FlattenException #33916

Closed
wants to merge 3 commits into from

Conversation

@alexander-schranz
Copy link
Contributor

commented Oct 8, 2019

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR symfony/symfony-docs#...

I'm currently trying to test 4.4-dev and did get over this issue when using FosRestBundle.
I also created a fix there but think this should not break in 4.4.
The old FlattenException did implement a create function.

…FlattenException
Copy link
Member

left a comment

Hi @alexander-schranz thanks for taking care of this, but I think we already have changed all the calls from FlattenException::create() to FlattenException::createFromThrowable() in 4.4 codebase. If your project or 3rd-party bundles use FlattenException::create() from Debug component for something, it must require symfony/debug package as a hard dependency instead of relying on it as a transitive one.

Thus, you'll be able to migrate without BC break. See discussion about this topic here #32873 (comment)

I'm 👎 on these changes.

@alexander-schranz

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

@yceruto Thank you for clarification. I still would add the function as it don't really hurt if its there and for people who relying on this don't get hurt when they update from 4.3 and 4.4.

return static::createFromThrowable($exception, $statusCode, $headers);
}
public static function createFromThrowable(\Throwable $exception, int $statusCode = null, array $headers = []): self

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Oct 9, 2019

Member

what about renaming this one to create instead? that would make the most sense to me as the current name is due to history - but this is a new component now

This comment has been minimized.

Copy link
@alexander-schranz

alexander-schranz Oct 9, 2019

Author Contributor

But if somebody did use createFromThrowable which did also exist on the Debug FlattenException it will get there an error. I think in 4.4 we still should provide both functions.

@stof

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

I don't see how this help at all regarding BC. If you want to migrate from one FlattenException to the other one, you have to change the class name used for the static call, not only the name of the method. Using the same method name in both cases will not remove the need for a if in FOSRestBundle. So I'm -1 on this PR.

@alexander-schranz

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2019

@stoff its not about migrating from one to another its a bc break which break current projects but want to move the discussion to an issue #33929 as there is a related #33918 which I think can be fixed by changing where the new FlattenException extend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.