cuny-academic-commons / commons-in-a-box

Commons In A Box - A suite of community and collaboration tools for WordPress, designed especially for academic communities
http://commonsinabox.org
72 stars 14 forks source link

Error on Site Creation #433

Closed beckej13820 closed 1 year ago

beckej13820 commented 1 year ago

I'm having a new error on the Oneonta OpenLab when new sites are being created. From my uninformed guess, these errors seem to be happening based on the order that the template system is creating things, and one of my themes "Astra" is being a pain in the butt.

Screenshot 2023-03-28 at 1 57 11 PM

^This error is actually showing up in the site creation process when you click the "next step" on "step 2" of site creation process.

Is anyone else getting errors on course creation? Are there any steps I can take to give you a more complete view of the problem?

beckej13820 commented 1 year ago

It's worth noting that despite the error, the site gets created.

beckej13820 commented 1 year ago

Sorry for dribbling information in little by little. This error only interrupts the signup flow when running PHP 8. When using PHP 7.4, no error is displayed to students.

boonebgorges commented 1 year ago

I've just installed the Astra theme on a CBOX group-type template site, and I'm not seeing this issue.

I've never seen this specific error language before, but it appears that it's linked to the way that unserialize() is used on some object types. The tricky part about diagnosing is that WP internally uses unserialize(), especially in the context of options.

Are you able to collect a full backtrace for the error? Perhaps, on a dev site, you can enable higher-level error reporting https://phoenixnap.com/kb/php-error-reporting This will allow us to get a better sense of just what part of Astra is triggering the error, which will in turn indicate whether there's a workaround in CBOX.

beckej13820 commented 1 year ago

Hi Boone,

I was able to stop the error message by rolling back to PHP 7.4, as it was only showing up on PHP 8.0. That makes this a lower priority for me. I will try to turn on more PHP logging on a demo site and get you a more actionable report next week.

Thanks for your quick response on this, and I will be in touch.

beckej13820 commented 1 year ago

Hi Boone,

I have started to set up a fresh OpenLab to try to replicate this issues. It took me a couple attempts to get an error to be thrown.

  1. Installing Astra wasn't enough.
  2. I had to install astra, network activate it,
  3. Go to the portfolio template enable astra, change a few settings.
  4. Change the portfolio to a different theme other than Astra.
  5. Then I went and created a portfolio using the template I had edited.

When I did those steps, here was the PHP error:

Fatal error: Uncaught Error: The script tried to modify a property on an incomplete object. Please ensure that the class definition "Astra_Builder_Header" of the object you are trying to operate on was loaded before unserialize() gets called or provide an autoloader to load the class definition in /home/edbecksu/demo.ed-beck.com/openlab/wp-includes/formatting.php:5022

Stack trace: #0 /home/edbecksu/demo.ed-beck.com/openlab/wp-includes/formatting.php(5017): map_deep(Object(__PHP_Incomplete_Class), Object(Closure))

1 /home/edbecksu/demo.ed-beck.com/openlab/wp-includes/formatting.php(5017): map_deep(Array, Object(Closure))

2 /home/edbecksu/demo.ed-beck.com/openlab/wp-includes/formatting.php(5017): map_deep(Array, Object(Closure)) #3 /home/edbecksu/demo.ed-beck.com/openlab/wp-includes/formatting.php(5017): map_deep(Array, Object(Closure)) #4 /home/edbecksu/demo.ed-beck.com/openlab/wp-content/plugins/cbox-openlab-core/includes/group-sites.php(1821): map_deep(Array, Object(Closure))

5 /home/edbecksu/demo.ed-beck.com/openlab/wp-content/themes/openlab-theme/lib/group-funcs.php(905): cboxol_copy_blog_page(5)

6 /home/edbecksu/demo.ed-beck.com/openlab/wp-includes/class-wp-hook.php(308): openlab_save_group_site('')

7 /home/edbecksu/demo.ed-beck.com/openlab/wp-includes/class-wp-hook.php(332): WP_Hook->apply_filters(NULL, Array)

8 /home/edbecksu/demo.ed-beck.com/openlab/wp-includes/plugin.php(517): WP_Hook->do_action(Array)

9 /home/edbecksu/demo.ed-beck.com/openlab/wp-content/plugins/buddypress/bp-groups/actions/create.php(150): do_action('groups_create_g...')

10 /home/edbecksu/demo.ed-beck.com/openlab/wp-includes/class-wp-hook.php(308): groups_action_create_group('') #11 /home/edbecksu/demo.ed-beck.com/openlab/wp-includes/class-wp-hook.php(332): WP_Hook->apply_filters(NULL, Array)

12 /home/edbecksu/demo.ed-beck.com/openlab/wp-includes/plugin.php(517): WP_Hook->do_action(Array)

13 /home/edbecksu/demo.ed-beck.com/openlab/wp-content/plugins/buddypress/bp-core/bp-core-dependency.php(368): do_action('bp_actions')

14 /home/edbecksu/demo.ed-beck.com/openlab/wp-includes/class-wp-hook.php(308): bp_actions('')

15 /home/edbecksu/demo.ed-beck.com/openlab/wp-includes/class-wp-hook.php(332): WP_Hook->apply_filters(NULL, Array)

16 /home/edbecksu/demo.ed-beck.com/openlab/wp-includes/plugin.php(517): WP_Hook->do_action(Array)

17 /home/edbecksu/demo.ed-beck.com/openlab/wp-content/plugins/buddypress/bp-core/bp-core-dependency.php(445): do_action('bp_template_red...')

18 /home/edbecksu/demo.ed-beck.com/openlab/wp-includes/class-wp-hook.php(308): bp_template_redirect('')

19 /home/edbecksu/demo.ed-beck.com/openlab/wp-includes/class-wp-hook.php(332): WP_Hook->apply_filters(NULL, Array)

20 /home/edbecksu/demo.ed-beck.com/openlab/wp-includes/plugin.php(517): WP_Hook->do_action(Array)

21 /home/edbecksu/demo.ed-beck.com/openlab/wp-includes/template-loader.php(13): do_action('template_redire...') #22 /home/edbecksu/demo.ed-beck.com/openlab/wp-blog-header.php(19): require_once('/home/edbecksu/...')

23 /home/edbecksu/demo.ed-beck.com/openlab/index.php(17): require('/home/edbecksu/...')

24 {main} thrown in /home/edbecksu/demo.ed-beck.com/openlab/wp-includes/formatting.php on line 5022

boonebgorges commented 1 year ago

Thanks for the stack trace, @beckej13820 . Here's what's happening.

  1. During the clone process, CBOX-OL copies a source site's options. As part of this copying, CBOX-OL unserializes each copied option in order to swap out the source site's URL with the destination site's URL. See https://github.com/cuny-academic-commons/cbox-openlab-core/blob/52ecf61b41d76acdc6c729c81e5d895581770469/includes/group-sites.php#L1740
  2. The Astra theme stores PHP objects in its theme settings (in at least one place - Astra_Builder_Header).
  3. The clone process takes place within the context of the "root" or main site, where the Astra theme's libraries are not loaded. As such, PHP attempts to unserialize the Astra option, finds that a member must be a Astra_Builder_Header, but can't find a class definition for that class name. Thus the fatal error.

It's not great practice to store full objects in the database for this reason. See https://stackoverflow.com/questions/2010427/php-php-incomplete-class-object-with-my-session-data But this is a change that the Astra authors would have to make. And, in their defense, the current situation is not one that would come up very often at all - in almost all cases, you're only accessing the option in a context where Astra is active, and thus the classes are all defined.

Moreover, while it's not a great idea for Astra to do this, it's possible that other themes/plugins would do something similar, and would trigger a similar issue in the clone process. Again, it's coming up in the very specific and odd context of CBOX-OL, so we should probably come up with some sort of workaround here.

The workarounds are not obvious. One is to simply skip options that are known to be potentially problematic. This is a relatively quick fix, but (a) it only covers the cases we know about, and (b) it decreases the utility of cloning/site templates by skipping important options.

A more general approach is to ensure that option serialization only takes place in the context of the new site, rather than on the root blog. This approach will ensure that the necessary libraries are always loaded. It could take a couple different forms. One is to postpone the copying of options altogether, to a cron job or a background process. This might work, but it might also have strange side effects if the routine doesn't run properly, and it's also a fairly large change to our infrastructure. A more modest fix is to postpone only the URL search-replace routine. We'd copy the options during site creation as we currently do, then run a cron job or background process to do the swap-outs.

This is non-trivial, but I think we can probably do it for our next major version.

In the meantime, I'm afraid I don't have a good recommendation for a workaround. It just seems like Astra is not fully compatible with the site template system in CBOX-OL at the moment.

beckej13820 commented 1 year ago

Hi Boone,

While Astra is a theme on my site that I don't want to disable, because it is used by multiple sites, I'm wondering if I can reset the templates in a way that they don't have any of that junk markup in the table.

If I was looking at the site table in MySQL, do you think it would be possible to delete the Astra-specific stuff so that at the very least, I could stop this error?

boonebgorges commented 1 year ago

If I was looking at the site table in MySQL, do you think it would be possible to delete the Astra-specific stuff so that at the very least, I could stop this error?

My initial thought about this was "no", because Astra appears to keep all of its settings in a single row of the options table. Deleting or excluding that option would mean that no pre-configured theme settings would be copied over at the moment of site creation, which kinda defeats the purpose of a site template.

But I took a closer look at what Astra is actually storing in the options table. It turns out that the PHP objects are not being stored in the main astra-settings option, but instead in something called astra_partials_config_cache. As suggested by the name, this is row that's used only for performance reasons, and only in very specific cases (namely, when rendering Customizer partials; see https://github.com/brainstormforce/astra/blob/175820d84f133cfd99448a0a49ef8d68140da6b0/inc/customizer/class-astra-customizer.php#L283). As such, I think it's harmless to skip it during the template copying process.

I've introduced a filter that allows third-party tools or site owners to add options to the "skip" list in 8239e8. As a rule, I'm not a huge fan of adding theme- or plugin-specific items to the list that's hardcoded in CBOX - where would we draw the line? - but I guess it's harmless to do so in this case, so I've done it in ef0aedd. In my tests, these changes (specifically the latter) cause the error to go away.

beckej13820 commented 1 year ago

Boone, I might try it anyway.

What I might not have gotten across, is that this site template does not use the Astra theme it actually uses a different theme. But I had played with Astra before deciding on a different theme. So if I delete the settings, I will not miss them in the template because it isn't even the active theme.

boonebgorges commented 1 year ago

Ah, if you're not using the Astra theme, then you can safely delete any astra rows in the options table for that site.