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

Jump to bottom

1.13.1 (and earlier): PUM_Telemetry triggers PHP warnings in (CLI) cron #919

Closed
lkraav opened this issue Dec 16, 2020 · 14 comments
Closed

1.13.1 (and earlier): PUM_Telemetry triggers PHP warnings in (CLI) cron #919

lkraav opened this issue Dec 16, 2020 · 14 comments
Labels
type-bug
Milestone
v1.15

Comments

@lkraav
Copy link

lkraav commented Dec 16, 2020

Describe the bug

Periodically cron spams us:

Notice: Undefined index: open_sound in /home/convers/public_html/wp-content/plugins/popup-maker/classes/Telemetry.php on line 139
Notice: Undefined index: open_sound in /home/convers/public_html/wp-content/plugins/popup-maker/classes/Telemetry.php on line 142
Notice: Undefined index: open_sound in /home/convers/public_html/wp-content/plugins/popup-maker/classes/Telemetry.php on line 139
Notice: Undefined index: open_sound in /home/convers/public_html/wp-content/plugins/popup-maker/classes/Telemetry.php on line 140
Notice: Undefined index: open_sound in /home/convers/public_html/wp-content/plugins/popup-maker/classes/Telemetry.php on line 139
Notice: Undefined index: open_sound in /home/convers/public_html/wp-content/plugins/popup-maker/classes/Telemetry.php on line 140
Notice: Undefined index: open_sound in /home/convers/public_html/wp-content/plugins/popup-maker/classes/Telemetry.php on line 139
Notice: Undefined index: open_sound in /home/convers/public_html/wp-content/plugins/popup-maker/classes/Telemetry.php on line 140
Notice: Undefined index: open_sound in /home/convers/public_html/wp-content/plugins/popup-maker/classes/Telemetry.php on line 139
Notice: Undefined index: open_sound in /home/convers/public_html/wp-content/plugins/popup-maker/classes/Telemetry.php on line 140
Notice: Undefined index: open_sound in /home/convers/public_html/wp-content/plugins/popup-maker/classes/Telemetry.php on line 139
Notice: Undefined index: open_sound in /home/convers/public_html/wp-content/plugins/popup-maker/classes/Telemetry.php on line 140
Notice: Undefined index: cookies in /home/convers/public_html/wp-content/plugins/popup-maker/classes/Telemetry.php on line 105
Warning: Invalid argument supplied for foreach() in /home/convers/public_html/wp-content/plugins/popup-maker/classes/Telemetry.php on line 105
Notice: Undefined index: open_sound in /home/convers/public_html/wp-content/plugins/popup-maker/classes/Telemetry.php on line 139
Notice: Undefined index: open_sound in /home/convers/public_html/wp-content/plugins/popup-maker/classes/Telemetry.php on line 140
Notice: Undefined index: cookies in /home/convers/public_html/wp-content/plugins/popup-maker/classes/Telemetry.php on line 105
Warning: Invalid argument supplied for foreach() in /home/convers/public_html/wp-content/plugins/popup-maker/classes/Telemetry.php on line 105
Notice: Undefined index: open_sound in /home/convers/public_html/wp-content/plugins/popup-maker/classes/Telemetry.php on line 139
Notice: Undefined index: open_sound in /home/convers/public_html/wp-content/plugins/popup-maker/classes/Telemetry.php on line 140

Site information

Popup Maker version: 1.13.1

WordPress version: 5.6

PHP version: 7.4

Expected behavior

Telemetry class should improve cron compatibility.

Current behavior

Probably some variables are not prepared for cron context.

Steps to reproduce

  1. Execute cron via wp --quiet cron event run --due-now

Errors

See bug description.

Additional context

Tracebacks
@danieliser
Copy link
Member

@lkraav strange, I've used cron recently and didn't see these. Not sure why they would be empty but I think some simple sanity checks there would be in order.

@danieliser danieliser added this to the v1.15 milestone Dec 16, 2020
@lkraav
Copy link
Author

lkraav commented Dec 16, 2020

strange, I've used cron recently and didn't see these. Not sure why they would be empty but I think some simple sanity checks there would be in order.

🤷‍♂️

This is on production server, so all config screens have been visited over time, for sure - if whatever has needed initialization.

I've sometimes also seen mistakes where new data objects storage routines only trigger on plugin activation, but not regular updates.

@danieliser
Copy link
Member

@lkraav well either way simple fixes I believe

@fpcorso
Copy link
Contributor

fpcorso commented Dec 18, 2020 •
edited

@lkraav @danieliser I looked into this a bit. When I wrote the telemetry class, I thought our $popup->get_settings(); would add defaults to settings that didn't exist. However, diving into it more now, I see it is pulled straight from the meta.

For example, we added the new open_sound feature so popups created and saved prior to that feature would not have that key. So, I am assuming some of your popups have not been saved since that feature was added resulting in that setting key not existing, hence the undefined index error. I'll throw a conditional around that to make sure it exists to fix that error which solves the majority of notices here.

Haven't figured out the other issue yet relating to the cookies foreach as I am not sure why the cookies key would be not enumerable. Still looking into it.

fpcorso added a commit that referenced this issue Dec 18, 2020
@fpcorso
Add conditional to ensure open_sound key exists
3e24e5b 
Since the opening sound setting was only recently introduced, some
popups can exist that do not have that setting yet resulting
in undefined index notices here.

Issue #919
@fpcorso
Copy link
Contributor

fpcorso commented Dec 18, 2020

@lkraav @danieliser I installed a version of PM that is 3 years old (1.5.8) and created a popup and then switched to the recent version and fired the Telemetry method. I did get a similar notice on the conditions key. So, it is possible that the popups here were created before a change in how cookies were implemented which would cause the key to not be set which is what I encountered with the conditions key in my test.

@lkraav Can you confirm that some of the popups on the site were created at least more than 2 years ago?

I'll go ahead and wrap the conditions and cookies in conditionals to test for that key too.

@danieliser
Copy link
Member

danieliser commented Dec 18, 2020 •
edited

@fpcorso since your pulling it all from settings (I think), just run $settings through wp_parse_args or our own PUM_Utils_Array:parse_allowed_args( $array, $default_allowed_args = [] ) instead of using per key conditionals.

So this would essentially prefill missing values at the start of the process and you can then safely expect those values to exist.

@lkraav
Copy link
Author

lkraav commented Dec 19, 2020

@lkraav Can you confirm that some of the popups on the site were created at least more than 2 years ago?

Yep, can confirm.

@fpcorso
Copy link
Contributor

fpcorso commented Dec 21, 2020

@danieliser Ah! There is actually already a better method for this. In our meta box, we use this to get settings with defaults which uses our fill_missing_defaults():

$settings = self::parse_values( $popup->get_settings() );

I should be able to use that line instead of my current one to resolve all of these. I'll test it.

@danieliser
Copy link
Member

@fpcorso As long as it uses something existing it should work fine.

That said the parse_allowed_args method only returns the keys you requested, so it has a lower memory footprint. Just something long term to think about in general usage.

@fpcorso
Copy link
Contributor

fpcorso commented Dec 21, 2020

Copying my comments from Slack:

Odd, I tried both wp_parse_args and PUM_Admin_Popups::parse_values here and both did not solve the undefined index errors. But returned the settings with the conditions key from my test popup still missing.

Interesting....stepping through the debugger now and the defaults() method returns an array that doesn't have triggers, cookies, or conditions as keys so doing any parsing off that won't work then.

@danieliser Whoa, the defaults() method only returns the defaults for checkbox types? https://github.com/PopupMaker/Popup-Maker/blob/master/classes/Admin/Popups.php#L1025
Is that right? Is that if ( $field['type'] == 'checkbox' ) { supposed to be there? We already have that check within the next line.

That defaults() method is what we use both creating the settings on the frontend and for the settings in the popup editor. I just checked in both places and can confirm the conditions key in my test popup are missing in both of those places too.

@danieliser
Copy link
Member

@fpcorso - Hmm, I do believe the JS form API for popup settings though pulls default values directly from field declarations as each field has a std key.

Also not 100% sure always filling keys with defaults is wise:

  1. Then we can't detect if they have set a value previously or not. IE detect if a migration is neccessary.
  2. If they actually unset it on purpose, we will be filling back a default that could consistently override.

Parsing for missing args at the time of usage is probably more preferable for a few reasons imho, primarily being that developers looking at code don't have to dig into other files/methods to find what is in the $settings array, they can see what they should expect right in the parse_args call, along with defaults if null is not sufficient.

Looks like it was last tweaked 3 years ago. Would have to look at more of the commits around that time to get more context: c786730

But we can try patching it, just not sure what fallout there would be this far down the road, nor am I sure we would re-utilize much of this functionality post rewrite.

@fpcorso
Copy link
Contributor

fpcorso commented Dec 22, 2020

@danieliser The JS popup settings uses $settings = wp_parse_args( $this->get_settings(), PUM_Admin_Popups::defaults() ); in get_public_settings() here: https://github.com/PopupMaker/Popup-Maker/blob/master/classes/Model/Popup.php#L217 which is then used to build the JS settings in PUM_Site_Assets: https://github.com/PopupMaker/Popup-Maker/blob/master/classes/Site/Assets.php#L301

So, even on the frontend, the keys are still missing.

So, to resolve this, we have a few options:

  1. Fix the defaults() method
  2. Create a new defaults() method that doesn't have the error and deprecate the old one
  3. Wrap the foreach's in conditionals in the Telemetry code

@danieliser
Copy link
Member

@fpcorso per slack I think simplest is remove the if statement as it seems like a leftover.

fpcorso added a commit that referenced this issue Dec 22, 2020
@fpcorso
Parse settings in telemetry to fill defaults
78952df 
Fix issue #919
@fpcorso
Copy link
Contributor

fpcorso commented Dec 22, 2020

@lkraav This has been fixed in a recent commit in develop. This should make it out in our 1.15 release. Thanks for reporting!

@fpcorso fpcorso closed this as completed Dec 22, 2020
@fpcorso fpcorso mentioned this issue Jan 8, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type-bug
Projects
None yet
Milestone
v1.15
Development

No branches or pull requests
3 participants
@lkraav @danieliser @fpcorso