Skip to content

Fix bc compatiblity between ErrorRenderer FlattenException and Debug FlattenException #33916

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

Closed
wants to merge 3 commits into from

Conversation

alexander-schranz
Copy link
Contributor

@alexander-schranz alexander-schranz 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.

Copy link
Member

@yceruto yceruto left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

@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.

@nicolas-grekas nicolas-grekas added this to the next milestone Oct 9, 2019

return static::createFromThrowable($exception, $statusCode, $headers);
}

public static function createFromThrowable(\Throwable $exception, int $statusCode = null, array $headers = []): self
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Member

stof 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
Copy link
Contributor Author

@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.

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
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.

5 participants