pivvenit / acf-composer-bridge

Provides a static composer repository that references all Advanced Custom Fields Pro versions
MIT License
47 stars 8 forks source link

Compatibility with WPML (and maybe other plugins) #8

Closed vyskoczilova closed 4 years ago

vyskoczilova commented 4 years ago

Hi, I'm back on the #7 issue, checked how the ACFML (WPML plugin for ACF) is checking presence of ACF, it's not ideal and colleague of mine working in there said that they might have changed it.

I was thinking about class_exists('ACF'). However, at least in my case when using composer within a theme, not in Bedrock I haven't found a way how to do it since plugins are called before themes :( Maybe you would get an idea?

Currently, they are using this function:

private function is_acf_active() {
    $active = false;

    $active_plugins = get_option( 'active_plugins' );

    $active_network_plugins = array();
    if ( function_exists( 'wp_get_active_network_plugins' ) ) {
        $active_network_plugins = wp_get_active_network_plugins();
    }

    $all_plugins = array_merge( $active_plugins, $active_network_plugins );

    if ( is_array( $all_plugins ) ) {
        foreach ( $all_plugins as $plugin ) {
            if ( stristr( $plugin, '/acf.php' ) ) {
                $active = true;
                break;
            }
        }
    }

    return $active;

}
Qrious commented 4 years ago

I would suggest you do not modify the source code of ACFML if possible. So i was thinking you could simply write a fake plugin with a acf.php as filename, to make their test pass? That works in case their other calls to ACF functions are wrapped within wordpress filters (like wp_loaded).

Since i don't have a WPML license, i cannot test this suggestion myself, but i'm interested in hearing back from you.

vyskoczilova commented 4 years ago

@Qrious I mean - if I'll find a solution, WPML will merge it officially to the ACFML.

I think can fake the plugin to pass the test, but that's quite a lot of hassle (and plugins would be spoiled with a fake plugin, maybe mu-plugins) but that's probably the cleanest way I see.

I could create a test site with WPML and share it with you.

Qrious commented 4 years ago

@vyskoczilova Ah, in case they merge the fix, they should simply start performing their work and check the existance of ACF using class_exists('ACF') in the wp_loaded event.

Disclaimer: I'm not familiar with their internals, so i'm not sure whether they can be savely moved to the 'wp_loaded' phase.

create a check like this (typed here, so it might contain compile errors):

<?php
add_action('wp_loaded', function() {
  $acfExists = class_exists('ACF');
  if (!$acfExists) {
    return; 
  }
 // Acf is loaded, do all of the plugins other work here
});

EDIT: wp_loaded is an aciton, so use add_action

vyskoczilova commented 4 years ago

That's a clever fix, I will pass it further with this thread link :)

kkarpieszuk commented 4 years ago

Hello, Konrad here, the author of ACFML

I have been pinged about this issue. I read it and I agree, for sure I will rewrite the plugin to check class existance instead what it does now.

This will be done probably inACFML 1.8.0 (the sprint for nex version, 1.7.0 is closed now and we will be releasing thsi version soon, I can't add anything now) so for time being the trick with fake acf.php is some idea.

Note to myself: ticket id in our system is ACFML-265

stale[bot] commented 4 years ago

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

kkarpieszuk commented 4 years ago

Hello dear bot ;) If this is question to me like I said, I will handle this in our internal bug tracking system. Is it up to this repository maintainer whether he would keep this ticket open or closed.

Qrious commented 4 years ago

I will close this issue, as it is not an issue with our ACF installer/composer bridge. @kkarpieszuk could you please comment on this issue once you have it resolved (with the version number that fixed it) to help any future visitors? Thanks in advance.

kkarpieszuk commented 4 years ago

hello, this is 99% sure it will be in release 1.8.0

we decided to not hook into wp_loaded but wpml_loaded which is fired just after after_setup_theme - this way ACFML has access to WPML API and any other code coming from themes and plugins

blanksoul commented 3 years ago

hello, this is 99% sure it will be in release 1.8.0

we decided to not hook into wp_loaded but wpml_loaded which is fired just after after_setup_theme - this way ACFML has access to WPML API and any other code coming from themes and plugins

Hi @kkarpieszuk in ACFML 1.8.1 that hook (wpml_loaded) seems not to work as expected. We have a quite small instance of WP (just WPML, ACF, ACFML) and a custom theme based on Sage and the only way to make "Translation preferences" field to show up is to revert the hook in wpml-acf.php to

add_action( 'wp_loaded', [ $acfml, 'init_worker' ] );

otherwise the is_acf_active return false and ACF field translation can't be customized.

The things that seems strange to me is that translation preferences for ACF fields are properly working in WPML->Settings page (I didn't dive into the code though, so maybe WPML makes different checks).

kkarpieszuk commented 3 years ago

Hi @blanksoul sorry for the late reply.

Could you try to add into your solution something like (or equivalent)

if ( class_exists( 'WPML_ACF' ) {
  add_action( 'wp_loaded', [  'WPML_ACF', 'init_worker' ] );
}

If this is not possible, could you provide somehow site dump where you see an issue?

blanksoul commented 3 years ago

Hi @kkarpieszuk, no problem.

The code you provided throws an error while trying to call init_worker statically.

Uncaught Error: Using $this when not in object context

I'm sure that WPML_ACF exists though and init_worker function works properly, the only issue seems related to the moment init_worker is called.

Following the extended snippet of wpml-acf.php with my changes:

$acfml_dependencies_factory = new WPML_ACF_Dependencies_Factory();
$acfml = new WPML_ACF( $acfml_dependencies_factory );
if ( $acfml ) {
        //add_action( 'wpml_loaded', [ $acfml, 'init_worker' ] );
        add_action( 'wp_loaded', [ $acfml, 'init_worker' ] );
        $acfml_dependencies_factory->create_requirements();
}

My context:

Further (useful?) details: I'm sure wpml_loaded action is fired but seems to be called too early and is_acf_active() (that checks if ACF class exists) return false.

Let me know if there is some other information that can be helpful ;)

kkarpieszuk commented 3 years ago

what if instead of

add_action( 'wp_loaded', [  'WPML_ACF', 'init_worker' ] );

you would have

add_action( 'wp_loaded', [  WPML_ACF::class, 'init_worker' ] );

?

I am affraid the team would not agree to change it back to hook into wp_loaded as your setup is quite unusual. But I have suggestion: you can add the code "extended snippet of wpml-acf.php" somewhere in your part of the code (for example functions.php), maybe only change commented line

//add_action( 'wpml_loaded', [ $acfml, 'init_worker' ] );

into remove_action to not call worker twice

remove_action( 'wpml_loaded', [ $acfml, 'init_worker' ] );
blanksoul commented 3 years ago

Unfortunately the alternative you provided return the same error.

Thinking thoroughly about my scenario, I think is quite normal that init_worker hooked to wpml_loaded action (inside ACFML plugin) is returning false about the existance of ACF class. When ACF is included inside the theme (through functions.php) the ACF class doesn't exist at plugin loading time.

This should be the order (please correct me if I'm wrong):

  1. WPML init function is called at plugins_loadedaction
  2. wpml_loadedaction run, and ACF doesn't exist (in my scenario)
  3. theme is setup
  4. functions.php code is run (and here we include ACF)

@kkarpieszuk introducing my "extended snippet" inside functions.php is probably the correct solution atm and makes things working as expected without making changes into wpml-acf.php file. ;)

IMHO though, since official website provides instructions on how to include ACF inside a theme (or plugin), wouldn't be more correct - and working in every scenario - to hook init_worker back to wp_loaded action? After all ACFML is plugin for WPML and ACF and should comply with them ;)

@kkarpieszuk once again thanks for your help.