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

fix compatibility with iThemes Sync #3989

Closed
mathetos opened this issue Feb 6, 2019 · 2 comments

Comments

Projects
None yet
5 participants
@mathetos
Copy link
Member

commented Feb 6, 2019

Feature Request

User Story

As a Give user who also depends on iThemes Sync for supporting my clients running Give, I want to be able to Sync my premium Give addons using Sync without problem.

Currently, Give's addons cannot be updated with Sync without a workaround.

The iThemes team gave us details on why this is happening, and it has to do with running our Email Notifications class action on init. If we instead used admin_init it would be resolved. But that needs testing.

Here's FULL details from iThemes Sync dev Lew Ayotte:

Basically, the plugin is resulting in a fatal error, this is what I saw in the logs:

PHP Fatal error: Call to undefined function give_get_current_setting_section() in /home/lew/public_html/wp-content/plugins/give/includes/admin/emails/abstract-email-notification.php on line 186


`give.php` includes this file: 

`require_once GIVE_PLUGIN_DIR . 'includes/admin/emails/class-email-notifications.php';`

Which includes the abstract:

`require_once GIVE_PLUGIN_DIR . 'includes/admin/emails/abstract-email-notification.php';`

Now, `class-email-notifications.php`, when it's loaded, it immediately runs `Give_Email_Notifications::get_instance()->load();` (bottom of the file). Which adds an early init hook. The class, on `init` includes several other email classes via the `add_emails_notifications()` method:

include GIVE_PLUGIN_DIR . 'includes/admin/emails/class-new-donation-email.php',
include GIVE_PLUGIN_DIR . 'includes/admin/emails/class-donation-receipt-email.php',
include GIVE_PLUGIN_DIR . 'includes/admin/emails/class-new-offline-donation-email.php',
include GIVE_PLUGIN_DIR . 'includes/admin/emails/class-offline-donation-instruction-email.php',
include GIVE_PLUGIN_DIR . 'includes/admin/emails/class-new-donor-register-email.php',
include GIVE_PLUGIN_DIR . 'includes/admin/emails/class-donor-register-email.php',
include GIVE_PLUGIN_DIR . 'includes/admin/emails/class-donor-note-email.php',
include GIVE_PLUGIN_DIR . 'includes/admin/emails/class-email-access-email.php',


The all get their `init()` methods called and they all extend the `abstract-email-notification.php` (`Give_Email_Notification` abstract class).

The Give_Email_Notification abstract class, has a method `setup_filters()` that checks for `is_admin()`... but by the `init` stage, Sync has set `IS_ADMIN` to `true`. So, in `setup_filters()` it tries to run the helper function that isn't loaded until is_admin is actually loaded, but the methods are being called on `init()`. There is an `admin_init` hook, you might be able to use, instead.

In the `includes/admin/emails/class-email-notifications.php` when I set the `load()` method to use `admin_init` instead of `init` to this... Sync works properly:

public function load() {
add_action( 'admin_init', array( $this, 'init' ), -1 );
}


But I have no idea how that'll affect your plugin.

Possible Solution

My suggestion is to consider just upadting that action hook because it might be more efficient anyway since our Email Class doesn't need to run on the front-end at all (which init does) so we should be able to rely on admin_init reliably (hopefully).

Acceptance Criteria

  • Hook is updated.
  • All email functionality continues to work exactly as intended
@lewayotte

This comment has been minimized.

Copy link

commented Feb 7, 2019

Let me know if y'all have any questions.

@DevinWalker

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

Research Findings

I dug into this a bit today and am finding it difficult to find a compatible solution with iThemes Sync without an extensive refactor of how we initialize our emails. The issue is that our plugin uses init early so add-ons like Recurring can add their add-on specific email functionality to the mix.

iThemes Sync Setting WP_ADMIN to true

private function set_is_admin_to_true() {
		if ( defined( 'ITHEMES_SYNC_SKIP_SET_IS_ADMIN_TO_TRUE' ) && ITHEMES_SYNC_SKIP_SET_IS_ADMIN_TO_TRUE ) {
			return;
		}
		
		if ( ! defined( 'WP_ADMIN' ) ) {
			define( 'WP_ADMIN', true );
		}
	}

I'm not certain why the plugin is setting WP_ADMIN to true, but I'm assuming it has to do with the admin-related features the Sync Dashboard allows. I'm also not certain what setting the workaround constant ITHEMES_SYNC_SKIP_SET_IS_ADMIN_TO_TRUE to false means for loss in functionality. However, I do know that this is the cause of the issue.

Next Steps

Changing the action hook we use to init to admin_init causes issues with frontend emails being set properly, such as the all important donation receipt and admin notification emails. If you look into the commit history on the issue/3989 branch you'll see I tried to hook setup_filters with admin_init rather than calling it directly within load() within abstract-email-notification.php however by this point WP_ADMIN is already true so the same result occurs with Sync failing.

It's hard for me to justify spending much more time on this issue because our plugin, as far as I know, works fine with ManageWP, InfiniteWP, and others. I'd like @ravinderk to spend 1 hour on this to see if he can figure out a simple solution that works.

@ravinderk ravinderk referenced this issue Mar 25, 2019

Merged

fix compatibility with iThemes Sync #3989 #4059

3 of 3 tasks complete

DevinWalker added a commit that referenced this issue Mar 25, 2019

Merge pull request #4059 from impress-org/issue/3989v2
fix compatibility with iThemes Sync #3989
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.