Skip to content

[WebProfilerBundle] Add missing Javascript function to prevent undefined function #48037

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
wants to merge 5 commits into from
Closed

[WebProfilerBundle] Add missing Javascript function to prevent undefined function #48037

wants to merge 5 commits into from

Conversation

PhilETaylor
Copy link
Contributor

Q A
Branch? 5.4+
Bug fix? yes
New feature? no
Deprecations? no
License MIT

So.... To replicate create a demo app with Symfony 5.4, 6.1, 6.2....

composer create-project symfony/symfony-demo mySymfony54 1.8.0
cd mySymfony54
symfony serve
# Visit http://127.0.0.1:8000/en/blog/ to confirm its working
# Edit BlogController.php and throw an \Exception('test') in the index function

The result is a Javascript error in the console log:

TypeError: Sfjs.createFilters is not a function. (In 'Sfjs.createFilters()', 'Sfjs.createFilters' is undefined)

Screenshot 2022-10-28 at 23 32 20

Now, Javascript in the profiler is loaded from two places (and each of these files has a header alerting you to that fact and that changes in one should be considered for the other too)

Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig (has no Sfjs.createFilters function)
Symfony/Component/ErrorHandler/Resources/assets/js/exception.js (Has a Sfjs.createFilters function)

Both files will only run their definition of the Sfjs if the typeof Sfjs === 'undefined' is true... so if one loads before the other then the latter will not redefine the Sfjs

However, what is happening is that the base_js.html.twig is defining the Sfjs object before exception.js gets a chance - and because the Sfjs.createFilters function is not in base_js.html.twig its not defined and so the error message tells us that.

This PR duplicates the method into the base_js.html.twig version of theSfjs object so that it can be called from the code (at the bottom of) exception.js and thus the console.log error message is now gone.

The code provided in this PR is not my code, its direct copy of the code from exception.js

@carsonbot carsonbot added this to the 5.4 milestone Oct 28, 2022
@MatTheCat
Copy link
Contributor

The issue rather comes from the fact base_js.html.twig redefines Sfjs if Sfjs.loadToolbar is undefined.

So if exception.js runs first it will define a version of Sfjs with createFilters and no loadToolbar. Problem is, it calls createFilters on DOMContentLoaded. So when base_js.html.twig kicks in, it redefines Sfjs with loadToolbar and no createFilters. Then DOMContentLoaded is triggered and it tries to call createFilters.

I don’t even know why there are two flavors of Sfjs?

@PhilETaylor
Copy link
Contributor Author

lol you literally just paraphrased what I had already said :)

I don’t even know why there are two flavors of Sfjs?

I guess you could show exceptions with component/ErrorHandler without the WebProfilerBundle being installed, thus exception.js would be the only code running.

@MatTheCat
Copy link
Contributor

Quite the opposite in fact: you’re saying base_js.html.twig is defining Sfjs before exception.js but it’s wrong because in that case createFilters would not be called and there would be no error.

Given createFilters targets [data-filters] and the only one exists in the translation collector, I’m not even sure it serves any purpose.

@PhilETaylor
Copy link
Contributor Author

PhilETaylor commented Oct 29, 2022 •
edited
Loading

I’m not even sure it serves any purpose.

I did think that, but I like to assume everything in Symfony has a reason to be there and that the code it mostly bug free. Maybe. I assume too much :)

So you would propose to remove calls to this function in the exception.js? In fact I think that's the better solution because <script>Sfjs.createFilters();</script> is called in translation.html.twig itself.

but it’s wrong because in that case createFilters would not be called and there would be no error.

The calls to the function are outside the "if undefined" block, and so they do get called.

@PhilETaylor PhilETaylor requested a review from yceruto as a code owner October 29, 2022 10:36
@PhilETaylor
Copy link
Contributor Author

So you would propose to remove calls to this function in the exception.js? In fact I think that's the better solution because <script>Sfjs.createFilters();</script> is called in translation.html.twig itself.

PR updated now to just that change.

@MatTheCat
Copy link
Contributor

MatTheCat commented Oct 29, 2022 •
edited
Loading

If createFilters is only called when it’s not defined it can be removed with all of its invocations and the related markup.

@MatTheCat
Copy link
Contributor

I think every occurrence of table-filters or table-filter attributes in HTML or CSS can be removed 🤔

Maybe you could change the PR’s title too!

@nicolas-grekas
Copy link
Member

Friendly ping @PhilETaylor

@PhilETaylor
Copy link
Contributor Author

Friendly ping @PhilETaylor

Thanks. Yup I know I suck :) I have this one and the Doctrine profiler PR to revisit, it's still on my plate to do both. Sorry for the delay.

@nicolas-grekas
Copy link
Member

Closing as this stalled. Please resubmit when you can. Or maybe @MatTheCat if you're up to?

@MatTheCat
Copy link
Contributor

I opened #50108 because we missed the big picture in this PR 😅

@MatTheCat MatTheCat mentioned this pull request May 2, 2023
Merged
fabpot added a commit that referenced this pull request May 5, 2023
@fabpot
bug #50213 [ErrorHandler] Prevent conflicts with WebProfilerBundle’s …
cd158aa 
…JavaScript (MatTheCat)

This PR was merged into the 5.4 branch.

Discussion
----------

[ErrorHandler] Prevent conflicts with WebProfilerBundle’s JavaScript

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #50108
| License       | MIT
| Doc PR        | N/A

ErrorHandler’s `exception.js` and WebProfilerBundle’s `base_js.html.twig` both expose a global `Sfjs` variable, which create conflicts (see linked issue e.g.).

As the exception page
- does not make use of `Sfjs`
- is not extensible

I updated `exception.js` to not create any global scope variable. I also removed `DOMContentLoaded` listeners as the script is run inline at the end of the document.

This PR stems from #48037.
Hiding whitespaces will produce a much more readable diff.

Commits
-------

e331b28 Do not expose `Sfjs` as it is unused and conflicts with WebProfilerBundle’s
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yceruto yceruto Awaiting requested review from yceruto yceruto is a code owner

Assignees
No one assigned
Labels
Bug Status: Needs Review WebProfilerBundle
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
4 participants
@PhilETaylor @MatTheCat @nicolas-grekas @carsonbot