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

Tweak - Allow Allow 3rd parties to suppress the "invalid meta" PHP notice #347

Open
daigo75 opened this issue Mar 17, 2020 · 5 comments · May be fixed by #348
Open

Tweak - Allow Allow 3rd parties to suppress the "invalid meta" PHP notice #347

daigo75 opened this issue Mar 17, 2020 · 5 comments · May be fixed by #348

Comments

@daigo75
Copy link

daigo75 commented Mar 17, 2020

Currently, class Puc_v4p9_Metadata raises a PHP notice when the response from the update server doesn't contain what the client library expects, such as the name and version entries. This notice can be superfluous, as in many cases there's already an error handling system in place to deal with this condition.

The notice is automatically hidden when the WP_DEBUG constant is not defined, but many live sites keep that notice on, causing the notice to appear in log files or to be shown to visitors. Adding a filter to allow 3rd parties to explicitly suppress the notice will allow to hide that notice as needed.

daigo75 pushed a commit to aelia-co/plugin-update-checker that referenced this issue Mar 17, 2020
@YahnisElsts
Copy link
Owner

[...] in many cases there's already an error handling system in place to deal with this condition.

Could you elaborate on this? Do you mean that many plugin and theme developers expect their update server to return valid JSON that is missing certain entries, and they've designed their code to handle that case? This seems unlikely to me.

@daigo75
Copy link
Author

daigo75 commented Mar 17, 2020

I worked on at least two projects where the call to the update endpoint returns a JSON without that information, because it's used for premium products. If a valid licence is not found, the JSON is valid, but it doesn't contain any plugin meta. That's different from the update server example, which always return the plugin meta, but removes the download URL.

In other words, the call to "get latest version" may return a response that is valid in such context, but doesn't contain the expected meta. That condition is handled gracefully, therefore there is no need to raise a notice in that case.

In addition to that, both the client and the server plugins I worked on use their own logging system, instead of relying on the trigger_error() function. That makes the notice redundant, as the logging has already been performed.

@YahnisElsts
Copy link
Owner

If the server response doesn't contain any plugin meta, it doesn't seem like a valid update API response even if it contains syntactically valid JSON. Could you perhaps make it return an error instead, like a 403 Forbidden error to indicate that the license is not valid? The response body can still contain JSON if you need to pass some information to the plugin.

The update checker already contains some code to handle non-200 responses and that seems like a better place to deal with this issue than the metadata class.

@daigo75
Copy link
Author

daigo75 commented Mar 17, 2020

In this specific case, that might not be that easy. An "invalid licence" is returned with a response code of 200, because it's considered a successful request. It all works fine, the only issue is that PHP notice, which is unnecessary to anyone except the developers. In any case, I will propose the change on the response header, maybe it will work too.

In general, the PHP notice is useful for debugging, and it's only shown when the WP_DEBUG constant is set. The issue is that such constant is often enabled in production, so we can't rely upon it being disabled. That's why I have a separate "debug mode" for each component I develop. It allows to enable that mode only for the ones being tested, without polluting the PHP log with unexpected errors.

@YahnisElsts
Copy link
Owner

I wouldn't be opposed to the idea of adding a "debug mode" filter. The update checker already has something similar in the form of the $debugMode property of the Puc_v4p9_UpdateChecker class. You can set it to false to override WP_DEBUG settings and disable some notices, but it currently doesn't affect metadata parsing.

Still, I think this kind of thing should happen in the update checker class, not in the metadata parser. Maybe the parser could call the triggerError method on the PUC instance instead of directly triggering a notice...

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 a pull request may close this issue.

2 participants