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

Use Strict. Expose all the problems. #6509

Closed

Conversation

okonomiyaki3000
Copy link
Contributor

Hey, y'all, don't accept this PR, it breaks things. Actually, it exposes things that are already broken.

I've wrapped the whole thing in a closure and set it to use strict. Then, because some functions need to be in the global scope, I changed them from function statements to function expressions and attached them to window. Other than those changes, this file hasn't been changed.

Now go ahead and try to use it. You'll need to be in debug mode (so you get the uncompressed file) and then go to the admin and try to do some admin stuff. For example, in any list view you can try:

  • Use the 'select all' checkbox
  • Use a single-row checkbox
  • Publish or unpublish items

You'll probably want to have your browser's console open while you do this stuff. You'll likely notice that:

a) it doesn't work
b) the console shows some errors

This is here to start a discussion but I've also got most of the fixes already done which I'll be submitting later.

@wilsonge
Copy link
Contributor

I have something here that deals with the semi-colons #6106

@okonomiyaki3000
Copy link
Contributor Author

That's good. There are also several cases of undeclared variables.

Beyond what's exposed by 'use strict', most of this code violates Joomla's js style rules. Also, a lot of it could simply be better written.

@okonomiyaki3000
Copy link
Contributor Author

Oh, and line 37 is just plain wrong.

@dgrammatiko
Copy link
Contributor

That line came from #3484, but why do you say it’s wrong?

@okonomiyaki3000
Copy link
Contributor Author

Somehow it must have gotten changed since then. Look again.

@wilsonge
Copy link
Contributor

Michaels fixed line 37 directly in core!

@Fedik
Copy link
Member

Fedik commented Mar 21, 2015

@okonomiyaki3000 👍
there some more error that need to fix after that, please be aware 😉
you can see some, and copy from my another pull (#6357) Fedik@f670e86#diff-c1b1173fc1376e9afd63e8ec801c711aR102

}
};

}(Joomla));
Copy link
Member

Choose a reason for hiding this comment

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

also you can add here (window, document, Joomla) and on the top also,
then js minifier will minify all window, document, Joomla variables in the script body 😉

@okonomiyaki3000
Copy link
Contributor Author

@Fedik this PR fixes nothing and is not meant to fix anything. It only exposes some of what needs to be fixed. Your points are correct but I won't do anything about them here.

@Kubik-Rubik
Copy link
Member

#6512 merged, this PR can be closed. Thanks @okonomiyaki3000!

@Kubik-Rubik Kubik-Rubik closed this Jul 9, 2015
@okonomiyaki3000 okonomiyaki3000 deleted the core-problems branch January 25, 2016 05:55
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.

None yet

6 participants