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

Account for widget callbacks being array objects #93

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

westonruter
Copy link

Fixes #64.

See also support forum topic: https://wordpress.org/support/topic/php-recoverable-fatal-error-when-switching-amp-mode/

This implements a fix I proposed in another support topic: https://wordpress.org/support/topic/standard-template-mode-internal-error/#post-12547053

This fixes compatibility with the AMP plugin which wraps all the registered widget callbacks with an object that implements ArrayAccess.

@fellyph
Copy link

fellyph commented Jan 28, 2022

Hi Folks,

Any update about this pull request? The users still having problems with issue #64 https://wordpress.org/support/topic/php-recoverable-fatal-error-when-switching-amp-mode/

@medgedecastro
Copy link
Contributor

Hello there,

The developers tried to replicate the issue in one of our demo site. Unfortunately, we did not encounter the same error or issues.

For us to troubleshoot further, may we request for a staging site of the site with the issue? It might be related to other settings, plugins, and theme of the site.

Thank you,
Mej

@fellyph
Copy link

fellyph commented Feb 9, 2022

Hi @medgedecastro,

I will ask the user to share some information about his site, plugins and environment.

Thanks,
Fellyph

@hollisterca
Copy link

Hi @medgedecastro,
I am the one that has these errors in the log file.
You find some info here
What else do you need to know?
WP 5.9
Theme: GT, Version 2.8.6 by Graphene Themes Solutions
Widget Option: Version 3.7.11
AMP: Version 2.2.1 Settings: Template Mode, Standard
Under AMP - Validated URLs - for example on New Vegan French Toast
I get
A PHP fatal error occurred while validating the URL.
This may indicate either a bug in theme/plugin code or it may be due to an issue in the AMP plugin itself.
The error details appear below. If you are stuck, please search the support forum for possible related topics,
or otherwise start a new support topic including the error message,
the URL to your site, and your active theme/plugins. Please include your Site Health Info.

Uncaught Error: Object of class AMP_Validation_Callback_Wrapper could not be converted to string in /home/.../wp-content/plugins/widget-options/includes/widgets/extras.php:135
Stack trace:
#0 /home/.../wp-content/plugins/amp/includes/validation/class-amp-validation-callback-wrapper.php(144): widgetopts_sidebars_widgets()
#1 /home/.../wp-includes/class-wp-hook.php(307): AMP_Validation_Callback_Wrapper->__invoke()
#2 /home/.../wp-includes/plugin.php(189): WP_Hook->apply_filters()
#3 /home/.../wp-includes/widgets.php(1039): apply_filters()
#4 /home/.../wp-includes/widgets.php(702): wp_get_sidebars_widgets()
#5 /home/.../wp-content/themes/graphene/header.php(26): dynamic_sidebar()
#6 /home/.../wp-includes/template.php(770): require_once('/home/...')
#7 /home/.../wp-includes/template.php(716): load_template()
#8 /home/.../wp-includes/general-template.php(48): locate_template()
#9 /home/.../wp-content/themes/graphene/single.php(9): get_header()
#10 /home/.../wp-includes/template-loader.php(106): include('/home/...')
#11 /home/.../wp-blog-header.php(19): require_once('/home/...')
#12 /home/.../index.php(17): require('/home/...')
#13 {main}
  thrown

Location: /home/.../wp-content/plugins/widget-options/includes/widgets/extras.php:135

Can you take a look at the 2 commits from Weston? What else would you like to know?

@fellyph
Copy link

fellyph commented Feb 9, 2022

Hi @medgedecastro

The error is happening during the page validation, to reproduce the error follow those steps:

  1. Use AMP plugin on Standard or Transitional mode
  2. Add widget from the plugin on a page where there is an AMP version available
  3. In the AMP admin bar menu on the frontend, select "Validate URL"

Screen Shot 2022-02-09 at 21 32 53

@medgedecastro
Copy link
Contributor

We have pushed the suggested fixes. Thank you!

@hollisterca
Copy link

Very nice. I noticed. 3.7.12 looks good so far

@westonruter
Copy link
Author

@hollisterca But you've said the fix doesn't work?

@medgedecastro In looking at the code deployed to WordPress.org, I don't see the changes applied: https://plugins.trac.wordpress.org/browser/widget-options/tags/3.7.13/includes/widgets/extras.php#L129

I still see:

$id_base = is_array( $opts['callback'] ) ? $opts['callback'][0]->id_base : $opts['callback'];

And not not the changes suggested in this PR.

@hollisterca
Copy link

Just some feedback:
on my prev. comment. So far was only a few minutes into testing. V 3.7.12 did not upgrade automatically but that was just a versioning problem, or was it. in V3.7.13 the upgrade worked again but I have issues again, the same ones I original posted.
See feedback above from Weston.
Hope you @medgedecastro can take another look why the code suggestions did not make it into the release.
Thanks

@medgedecastro
Copy link
Contributor

Hi @hollisterca @westonruter

It seems that the changes we need were gone when @edbertguinto push some updates. We will work on it.

Thank you,

@medgedecastro
Copy link
Contributor

@hollisterca @westonruter

We have pushed the suggested fixes. Can you please confirm it is reflecting on your end.

@hollisterca
Copy link

@medgedecastro and @westonruter
now on Version 3.7.14
The fatal errors seem to be gone
in AMP I get now other issues

  • Invalid markup removed: 34
  • CSS Usage went from 15% to 90% on rechecking a few pages

@westonruter
Copy link
Author

@hollisterca That probably means it is working as expected. Previously validation could not be completed due to the fatal error, and so the amount of CSS was abbreviated and not all markup was rendered for AMP to validate.

@westonruter
Copy link
Author

If there is no longer a fatal error, then it seems the Widget Options issue is fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants