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

Suggest installing symfony/debug #619

Closed
wants to merge 1 commit into from
Closed

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented Jun 28, 2019

Q A
License MIT
Doc issue/PR symfony/symfony-docs#...

See symfony/symfony#32270

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

@yceruto yceruto changed the title Suggest install symfony/debug Suggest installing symfony/debug Jun 28, 2019
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

@OskarStark
Copy link
Contributor

But this isn’t a suggestion, it’s a hard dependency, isn’t it? If it exists it’s enabled, if not you cannot go further. Or am I wrong?

@@ -9,7 +9,11 @@
if ($_SERVER['APP_DEBUG']) {
umask(0000);

Debug::enable();
if (class_exists(Debug::class)) {
Debug::enable();
Copy link
Member

Choose a reason for hiding this comment

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

I understand wrapping this in an if and only running when it exists, but I don’t understand throwing an exception if it doesn’t exist.

@nicolas-grekas
Copy link
Member

On another recipe/PR, I suggested moving the Debug class to the new component. Applies here too.

@yceruto
Copy link
Member Author

yceruto commented Jun 29, 2019

Yes, that would solve the problem. I'll make the move, but that mean the Debug component is almost empty, does it make sense move all and deprecating it completely?

@fancyweb
Copy link
Contributor

fancyweb commented Jul 1, 2019

On another recipe/PR, I suggested moving the Debug class to the new component. Applies here too.

The Debug class calls the DebugClassLoader. So the ErrorCatcher would need to require symfony/debug... IMHO, the Debug class needs to stay in the Debug component or this component can be renamed to the Debug class loader component 😕

Or, since the DebugClassLoader usage in the Debug class is only DebugClassLoader::enable(), we could :

  • Deprecate the Debug component Debug class in 4.4 to remove it in 5.0.
  • Create a new class ErrorCatcher with the enable method in the ErrorCatcher component (copy pasted from the current Debug class).
  • We update the recipes to call DebugClassLoader::enable(); ErrorCatcher::enable(); in the if.
  • To solve the existing project update, we can include a file in the FrameworkBundle that would substitute the Debug class if it does not exist, do the calls but throw a deprecation to ensure users switch to the new behaviors for 5.0.

The problem with splitting is that if we have new debug behaviors, it would be better to have a single method like Debug::enable(); to be able to enable it by default and without user effort.
So we can also just add the symfony/debug dependency to the recipe so new projects are ready and for the existing ones, do my number 4 point of the above list. So at least symfony/debug is not a hard dependency of the FrameworkBundle as it feels wrong.

@yceruto
Copy link
Member Author

yceruto commented Jul 1, 2019

Closing in favor of symfony/symfony#32303

@yceruto yceruto closed this Jul 1, 2019
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.

5 participants