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

Don't need a closure, jQuery ready function is enough. Use strict. #12543

Conversation

okonomiyaki3000
Copy link
Contributor

@okonomiyaki3000 okonomiyaki3000 commented Oct 25, 2016

Pull Request for Issue # .

Summary of Changes

Remove the enclosure, use the concise jQuery ready function syntax, use javascript strict mode.

Testing Instructions

Go to the administrator (Isis) and open your browser's javascript console. You may see errors when performing any action that involves javascript (like using the menu) or just by loading a page.

This PR only exposes these errors. It does not fix them. The fix is in #12544, do not merge this PR. Merge that one.

Documentation Changes Required

Nope

@okonomiyaki3000
Copy link
Contributor Author

See #12544 for the fixes.

@okonomiyaki3000 okonomiyaki3000 force-pushed the use-strict-expose-errors branch from be00bcc to 1914619 Compare November 21, 2016 05:06
@ghost
Copy link

ghost commented Jan 12, 2017

@okonomiyaki3000 Tested like written in instructions, see with or without Patch Errors or not. So don't know what exact to look for.

@okonomiyaki3000
Copy link
Contributor Author

@franz-wohlkoenig It should not be difficult to find errors. Any page of the administrator should at least produce: Uncaught SyntaxError: Octal literals are not allowed in strict mode.

@ghost
Copy link

ghost commented Jan 13, 2017

I have tested this item 🔴 unsuccessfully on 1914619

Tested in J3.7.0-alpha2-nightly on different Isis-Menues, got no Uncaught SyntaxError: Octal literals are not allowed in strict mode. With and without PR got Warnings like getAttributeNode() shouldnt used. Use getAttribute().


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12543.

@okonomiyaki3000
Copy link
Contributor Author

Excellent. I think 'getAttributeNode()' has nothing to do with this file though. Where does your js console say it appears?

The 'octal literals' bug is actually caused by a line like this:

var cls = this.className.replace(/^.(chzn-color[a-z0-9-_]*)$.*/, '\1');

You will notice that there are no octal literals here. The problem is the replacement string. It should be '$1', not '\1'. This line still seems to be in 3.7 so I'm not sure why your browser is letting it go. May I ask which browsers you've tested this in?

Anyway, keep in mind that this PR is not meant to fix anything and is not expected to be merged. It is here as a reference to expose problems which normally go unnoticed because strict mode is not used. The goal of this patch is to make things break. The fixes are in #12544. That is the PR that should actually be merged.

@ghost
Copy link

ghost commented Jan 16, 2017

@okonomiyaki3000 Used Firefox 50.1.0, thanks for explanation of this PR. Will test #12544

@okonomiyaki3000 okonomiyaki3000 force-pushed the use-strict-expose-errors branch from 9e01ebf to 2392574 Compare January 17, 2018 14:17
@fancyFranci
Copy link
Contributor

I have tested this item ✅ successfully on 2392574

Now I'm getting Uncaught SyntaxError: Octal escape sequences are not allowed in strict mode.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12543.

Tchangue added a commit to Tchangue/joomla-cms that referenced this pull request Jul 23, 2018
@Tchangue
Copy link

@okonomiyaki3000 could you please look into: #21233 Thanks

@okonomiyaki3000
Copy link
Contributor Author

@Tchangue Yes, this PR has errors. That's actually the only point of it. It is not meant to be merged. It meant to expose the errors by invoking strict mode. The errors are actually fixed in #12544.

@okonomiyaki3000
Copy link
Contributor Author

@FPerisa You got errors. Which means the patch works because it's supposed to expose errors. But I think you're supposed to mark it as failed because we actually do not want errors. Yes, it seems to make no sense. The problem is that the errors are there already, they're just not reported because we're not in strict mode. So in fact, this PR should be Failed and the current Joomla code should also be Failed. #12544 is correct.

@kneisel
Copy link

kneisel commented Jul 24, 2018

I have tested this item ✅ successfully on 2392574

I think this issue can be closed together with #12544 because #12543 was only a patch to allow the test of #12544


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12543.

@ghost
Copy link

ghost commented Jul 24, 2018

Ready to Commit after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 24, 2018
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 24, 2018
@okonomiyaki3000
Copy link
Contributor Author

@franz-wohlkoenig Do not commit.

@okonomiyaki3000
Copy link
Contributor Author

@kneisel you can't have tested this patch successfully. It intentionally has errors.

@okonomiyaki3000 okonomiyaki3000 deleted the use-strict-expose-errors branch July 25, 2018 00:09
@ghost
Copy link

ghost commented Jul 25, 2018

@okonomiyaki3000 you mean set RTC back on Pending?

@ghost
Copy link

ghost commented Jul 25, 2018

sorry, this is closed.

@okonomiyaki3000
Copy link
Contributor Author

This is fine, it was closed without merging. As it should be. We're all fine here now.

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