-
Notifications
You must be signed in to change notification settings - Fork 156
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
Register scripts before being enqueued #356
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thank you!
@@ -130,6 +114,23 @@ public function get_label() { | |||
* @since 0.1-dev | |||
*/ | |||
public function login_enqueue_assets() { | |||
|
|||
wp_register_script( | |||
'fido-u2f-api', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is also referenced here:
two-factor/providers/class.two-factor-fido-u2f-admin.php
Lines 75 to 81 in 2b4ce5c
wp_enqueue_script( | |
'fido-u2f-admin', | |
plugins_url( 'js/fido-u2f-admin.js', __FILE__ ), | |
array( 'jquery', 'fido-u2f-api' ), | |
self::asset_version(), | |
true | |
); |
so we need to make sure this script is registered before it is enqueued there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yea, made a change to the PR to register on admin_enqueue_scripts
with a lower priority to make sure it gets registered prior to this. But when does login_enqueue_assets get called?
7f2f616
to
de6d035
Compare
Hi @kasparsd , Any plans to get this merged in the next release? |
Fixes #361. |
There appear to be a few code style issues reported here https://travis-ci.com/github/WordPress/two-factor/jobs/333298974 and a merge conflict with the latest changes introduced by the latest worked merged into Is it OK if I open a new pull request to fix these and also make sure the scripts are available on the login screen? |
Replacing this with #368 if that's OK. @rinatkhaziev, @cojennin Could you please code review #368? |
Registering the
fido-u2f-api
andfido-u2f-login
scripts in the__constructor
is generating the following notice:Notice: wp_register_script was called incorrectly. Scripts and styles should not be registered or enqueued until the wp_enqueue_scripts, admin_enqueue_scripts, or login_enqueue_scripts hooks.
It seems like these
wp_register_script
calls should be moved tologin_enqueue_assets
as that's where they're utilized.(Props @scottnelle for tracking this down)