8000 JavaScript error on all Exception pages with profiler · Issue #25894 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

JavaScript error on all Exception pages with profiler #25894

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.

Sign up for GitHub

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

Jump to bottom
Closed
weaverryan opened this issue Jan 23, 2018 · 10 comments
Closed

JavaScript error on all Exception pages with profiler #25894

weaverryan opened this issue Jan 23, 2018 · 10 comments

Comments

@weaverryan
Copy link
Member
weaverryan commented Jan 23, 2018 •
edited
Loading
Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 4.0 (but probably earlier)

When the profiler is present, the following JS error shows on every exception page:

screen shot 2018-01-22 at 9 25 54 pm

To repeat:

composer create-project symfony/skeleton exception_js_error
cd exception_js
composer require twig profiler
php -S 127.0.0.1:8000 -t public

Then go to any error page, like http://localhost:8000/foo.

It appears that the Sfjs.createTabs() (that executes the createTabs() method in base_js.html.twig of the WebProfilerBundle) is called twice. The first time works, but the second time, some classes (which were removed the first time) are missing.

It's possible this came from 1c595fc#diff-de00cc2fe56e59a48a4d26e7a0dccca3, but I'm very not sure - maybe @ro0NL can prove/disprove my theory. Updated - it doesn't look like that specific change was the problem - as changing back has no effect. Sorry for the noise.

Cheers!
@ro0NL
Copy link
Contributor
ro0NL commented Jan 23, 2018

See #24281 also ;)

@weaverryan
Copy link
Member Author

@ro0NL yea, that one looks suspicious... but huge :). I was hoping you could help me navigate the possible cause - I'm a bit lost in this area 😇

@ro0NL
Copy link
Contributor
ro0NL commented Jan 23, 2018

AFAIK it's historic, never worked. Dunno :)

General fix shouldnt be too hard.. assign/check a data-processed attr or so on both sides.

@weaverryan
Copy link
Member Author

@ro0NL So in your opinion, it's not some bigger "clash" between the profiler and exception page JavaScript? On the surface, if you look at the code in 1c595fc, it looks like both the exception page and the web debug toolbar JavaScript call the same Sfjs.createTabs(); function - i.e. so that it's called twice. But, my impression is probably that the Sfjs is actually two distinct variables (just with the same name) that exist independently on the page. Or am I wrong? And when we're on a page where both sets of JavaScript exist (i.e. error page + web debug toolbar), the Sfjs variable IS the same one instance.

@ro0NL
Copy link
Contributor
ro0NL commented Jan 23, 2018 •
edited
Loading

some bigger "clash" between the profiler and exception page

It is, yes. The exception page (TwigBundle) needs to work independent from the profiler (WebProfilerBundle). If you hit the exception page with profiler enabled then it conflicts.

Sfjs is actually two distinct variables

The PR was a first cleanup to reduce the big copy/paste of javascript. But yeah both truly provide a JS api, rooted by Sfjs. But it's copy/paste... in fact i believe when i faced the issue the tabs from the twig exception page we're actually rendered by the profiler JS, letting it's own JS crash 😀

If we can find a way to share frontend thingy's in general.. that be nice. I.e. the toggle links for logs in profiler is not available at the exception page. Maybe drop the JS features on the exception page? Gracefully enable if profiler is enabled?

@ro0NL
Copy link
Contributor
ro0NL commented Jan 23, 2018

In the long run.. it would be really cool if we can adapt a frontend framework/standard or so (basically #21050). So we can enhance the profiler/UI way more powerful with better UX and code. I.e. rich features a la #24263 could be done less cheap :-)

@ro0NL ro0NL mentioned this issue Feb 9, 2018
Closed
@stof
Copy link
Member
stof commented Feb 9, 2018

maybe the JS on the exception page should use a different variable for its JS to avoid clashing with the profiler (as they are meant to be usable separately or in parallel)

@ro0NL ro0NL mentioned this issue Feb 9, 2018
Merged
@ro0NL
Copy link
Contributor
ro0NL commented Feb 9, 2018 •
edited
Loading

@stof see #26119 i think handling it gracefully is the better fix opposed to renaming variables (which can only lead to more issues :))

edit: variable rename wont fix it anyway :) both code paths run

fabpot added a commit that referenced this issue Feb 12, 2018
@fabpot
bug #26119 [TwigBundle][WebProfilerBundle] Fix JS collision (ro0NL)
267bf4c 
This PR was merged into the 3.4 branch.

Discussion
----------

[TwigBundle][WebProfilerBundle] Fix JS collision

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes/no
| Fixed tickets | #25894
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->

<!--
- Bug fixes must be submitted against the lowest branch where they apply
  (lowest branches are regularly merged to upper ones so they get the fixes too).
- Features and deprecations must be submitted against the master branch.
- Replace this comment by a description of what your PR is solving.
-->

Commits
-------

da39e01 [TwigBundle][WebProfilerBundle] Fix JS collision
@fabpot fabpot closed this as completed Feb 12, 2018
@emschu
Copy link
emschu commented Feb 21, 2018

I can confirm this bug with current Symfony 3.4.4 version.
Does your fix affect this version?
Thank you.

@ro0NL
Copy link
Contributor
ro0NL commented Feb 21, 2018

@emschu AFAIK it will be in 3.4.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fabpot @weaverryan @stof @ro0NL @emschu
0