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

EZP-27290: Remove abstract on DebugTemplate to fix dumping logs in profiler #1957

Merged
merged 1 commit into from
Apr 27, 2017

Conversation

emodric
Copy link
Contributor

@emodric emodric commented Apr 14, 2017

https://jira.ez.no/browse/EZP-27290

Starting with symfony/symfony@eddecbd and in combination with legacy eZ templates, when an exception happens (even a silenced deprecation notice, which is converted to an exception) while rendering the template, one gets a message Cannot instantiate abstract class eZ\Bundle\EzPublishDebugBundle\Twig\DebugTemplate in the log, which ultimately breaks the profiler.

The issue comes from https://github.com/symfony/symfony/blob/master/src/Symfony/Component/VarDumper/Caster/ExceptionCaster.php#L201-L202, which presumes that every subclass of Twig_Template is instantiable.

Haven't found other way to solve this other than this. The fix is also compatible with Twig 2.0 (for when we enable it again in the repo).

@andrerom andrerom requested a review from bdunogier April 15, 2017 11:42
@andrerom
Copy link
Contributor

@lolautruche review ping

@emodric
Copy link
Contributor Author

emodric commented Apr 19, 2017

It turns out this is not related to eZ Legacy exclusivelly. The error basically occurs with any deprecation message that happens during rendering of a Twig template.

@bdunogier
Copy link
Member

would it make sense to fix it upstream ? Trying to instanciate abstracts doesn't sound right.

@emodric
Copy link
Contributor Author

emodric commented Apr 19, 2017

Well, as far as I know, Twig_Template was never meant to be extended. unserialize call in ExceptionCaster that's causing the issue is there from Symfony 2.8 era, it only now came to light as an issue, due to implementing changes to logger profiler.

@andrerom
Copy link
Contributor

andrerom commented Apr 19, 2017

is this feature (us extending it to add inline debug in html) needed anymore btw? doesn't Twig or TwigBundle by now have a similar out of the box feature?

@emodric
Copy link
Contributor Author

emodric commented Apr 19, 2017

@andrerom I'm not aware of it. Can you provide more details?

@andrerom
Copy link
Contributor

no :)

@emodric
Copy link
Contributor Author

emodric commented Apr 24, 2017

So, what can we do here?

Going in, or shall we look for other solutions?

@lolautruche
Copy link
Contributor

AFAIK Twig doesn't provide the same thing out of the box.
Yes Twig bundle provides a graph, but no delimiters like we provide (maybe we should propose it).

So I'm +1 to merge this in

Copy link
Contributor

@andrerom andrerom left a comment

Choose a reason for hiding this comment

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

but no delimiters like we provide (maybe we should propose it).

could do that in for instance 3.4 if they would be interested in such a feature. The buffering might be a problem, but maybe not.

@emodric
Copy link
Contributor Author

emodric commented Apr 24, 2017

@andrerom Need an issue?

@andrerom
Copy link
Contributor

andrerom commented Apr 24, 2017

@emodric yes please

@emodric emodric changed the title Remove abstract on DebugTemplate to fix dumping logs in profiler EZP-27290: Remove abstract on DebugTemplate to fix dumping logs in profiler Apr 24, 2017
@emodric
Copy link
Contributor Author

emodric commented Apr 24, 2017

@andrerom Done!

@andrerom
Copy link
Contributor

@bdunogier ? (the failure is unrelated, afaik it should be fixed if @emodric rebases)

@emodric
Copy link
Contributor Author

emodric commented Apr 25, 2017

@andrerom Rebased.

@emodric
Copy link
Contributor Author

emodric commented Apr 27, 2017

So, going in? :)

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

👍

@andrerom andrerom merged commit ec0a408 into ezsystems:6.9 Apr 27, 2017
@bdunogier
Copy link
Member

In my opinion, this should go into 6.7 (LTS). Any objection ?

@emodric
Copy link
Contributor Author

emodric commented Apr 27, 2017

@bdunogier I don't see why not, but 6.9 kernel is the first one that supports Symfony 3.x anyway, so maybe no need?

@andrerom
Copy link
Contributor

It was added to 6.7 in f3b9c53

kemoc pushed a commit to kemoc/ezpublish-kernel that referenced this pull request May 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants