Add function_exists( 'add_action' ) check to initialization file

[salcode]

Loading Action Scheduler via Composer

When loading Action Scheduler via Composer, I need to autoload vendor/prospress/action-scheduler/action-scheduler.php which Composer supports natively 😃 .

"autoload": {
    "files": [
        "vendor/prospress/action-scheduler/action-scheduler.php"
    ]
},

This works great when action-scheduler.php is loaded within the context of WordPress.

The Problem with Autoloading action-scheduler.php

If a project is also using a PHP based development tool (e.g. PHP_CodeSniffer (phpcs)), that tool runs outside of the context of WordPress. Because of this when vendor/prospress/action-scheduler/action-scheduler.php is autoloaded, a Fatal Error is thrown because add_action() is not a defined function ☹️ .

Suggested Solution

By adding a function_exists( 'add_action' ) check before calling add_action(), we could skip this fatal error when a PHP based development tool is run and action-scheduler.php is autoloaded via Composer.

Thanks for this great library (and regardless of how I load it) it is a big help on my current project.

Thanks!

[salcode]

For reference: this same problem occurs when loading the CMB2 library as a composer dependency (see CMB2 Issue 1270 Add function_exists( 'add_action' ) check to initialization file)

[johnbillion]

IMO this isn't the correct solution. A WordPress plugin should be able to assume that the WordPress plugin API functions are available, otherwise every plugin in the WordPress ecosystem needs to be updated with function_exists() calls to make it compatible with Composer autoloading.

The mistake is that your project is autoloading a WordPress plugin. By its nature a WordPress plugin entrypoint file will almost always have side effects (most commonly by adding actions and filters) instead of simply declaring symbols, which makes it unfit and inappropriate for autoloading in the context of Composer.

Most established Composer-based project frameworks recommend to load WordPress plugins as actual plugins or mu-plugins, instead of autoloading them directly via Composer, for exactly the reason you've identified. For example Bedrock has its own autoloader and any must-use plugins are loaded at the correct point instead of being directly autoloaded in the Composer configuration.

[salcode]

@johnbillion, I'm a big fan of your work and I really appreciate your input on this issue. I agree whole heartedly with much of what you've written, specifically I agree with the rule

A WordPress plugin should be able to assume that the WordPress plugin API functions are available

An additional rule I would propose is a library should never assume the WordPress plugin API functions are available.

Updated: An additional rule I would propose is a library should never hook into WordPress without being called.

Because Action Scheduler (and CMB2) exist as both a plugin and a library (See Action Scheduler Installation and CMB2 Installation), I see a conflict of these two rules.

The problem I've described in this issue (being unable to run PHPCS) occurs when we treat Action Scheduler as a library (rather than a plugin).

Proposed Solutions

1. Add the function_exists() check

In PR #304, I've created this proposed solution. While I recognize this is not an ideal solution, I think it is the most practical.

2. Create a Library and a Plugin

We could move all of the functionality (with the exception of the initial add_action()) into a new library. Then when using Action Scheduler as a library, you would include the library and call the initial add_action() from within your own code (e.g. in your plugin). As a plugin Action Scheduler would load the library and provide the initial add_action() call.

I see this is the purest programming solution however I don't think it is the most practical.

3. Remove Documentation on Using this as a Library

While I recognize removing any documentation regarding loading this code as a library seems extreme, I think if the project is going to be treated strictly as a plugin, the documentation should reflect this.

4. Document this Library Should Not be used with Composer

If we keep the documentation on loading this project as a library, we could document the problem with loading this library via Composer. I'm not a big fan of this approach as it feels like we're leaving something broken and documenting why it is broken.

Thank You for Your Consideration

I appreciate your time on this issue and while I would certainly like to see the function_exists() check added, regardless of outcome I will continue to appreciate all of the value Action Scheduler brings to the projects I work on. Thank you.

[johnbillion]

Ah-ha, now I agree with you that if we assume this is a library instead of a WordPress plugin then autoloading it ought to be possible with Composer.

That said, the bootstrap procedure of the library needs to work differently in that case so the only files that are autoloaded are ones with no side effect (ie. files that define functions and classes). The problem that introduces is moving the bootstrap process to the consumer, which should be a case of calling a function which sets up the actions and filters which are otherwise set up in the base plugin file (as you mention in point 2).

[thenbrent]

I see this is the purest programming solution however I don't think it is the most practical.

Agreed. What do you see as impractical about this approach though @salcode?

I think it would be relatively straightforward.

At first, I was concerned it would break backward compatibility and require a lot of plugins using it to change how they load it. However, existing plugins loading it as a library by including action-schedule.php would still work fine. It could be a "deprecated" method that is no longer documented, but it would continue to work.

The changeset would be quite minimal too. We'd just need to move action_scheduler_register_2_dot_2_dot_3() & action_scheduler_initialize_2_dot_2_dot_3() out of action-scheduler.php and into a new file (perhaps in a new class, like a ActionScheduler_Registrar class, and existing class, like ActionScheduler_Versions, or just a flat file for the two functions).

[salcode]

@thenbrent

Viewing the cost/benefit ratio, I'm not sure I see this as a good use of time. I see a number of costs (that would be borne by you as the maintainers, which I try to minimize when submitting a PR 😀), with not a significant amount of benefit over the current PR.

Costs

• create new repo for library
• publish new library on packagist.org
• move appropriate issues over to new repo
• any outstanding PRs would have to be re-done for the new repo
• moving forward there would be two repositories to manage for this project, rather than one
• modify this repo to load the library via Composer and hook in (note: currently this project does not have any required code dependencies (other than dev dependencies), which means currently someone can clone the repository and use the code without running Composer, however after this change, running Composer will be required. While I'm a big fan of Composer, this does increase the complexity of this project. I don't see this as a reason not to do it, but I think it is important to recognize this increase in complexity).

Benefits

The action-scheduler functionality could be loaded as a library via composer without automatically hooking in (thus solving the problem with other PHP tools that run outside of WordPress (e.g. PHP_CodeSniffer)).

From my perspective merging the current PR to add the conditional check for add_action() gives us these same benefits, with virtually no cost. While we may see issues in the future from the current architecture of hooking in as a library (e.g. if a PHP based tool has a function called add_action() or classes with the same name), I see these as unlikely and low risk since their impact should only occur outside of WordPress (i.e. breaking development tools, not breaking WordPress).

[thenbrent]

Ah thanks for the clarification @salcode, I completely misunderstood what you were saying, and thought you were referring to having the library & plugin use cases handled in the same codebase still. Specifically, I thought we could make action-scheduler.php used only for the plugin use case, and move everything not required for that out of there so it can still be loaded via composer or other libs.

From my perspective merging the current PR to add the conditional check for add_action() gives us these same benefits, with virtually no cost.

Yes, agreed. My only lingering concern is the gotcha it creates. Developers will likely (and rightly) expect AS to init itself when loaded via composer.

Out of curiosity, how are you loading AS when adding the function_exists( 'add_action' )?

[salcode]

@thenbrent

My only lingering concern is the gotcha it creates.
Developers will likely (and rightly) expect AS to init itself when loaded via composer.

The current behavior will not change for developers. Currently, if you require Action Scheduler via composer as a dependency it will not init itself. To get Action Scheduler to init itself, you need to specifically autoload action-scheduler.php (more information on this below).

Out of curiosity, how are you loading AS when adding the function_exists( 'add_action' )?

I've added action-scheduler.php as a file autoload with the following (more information about doing something similar with CMB2 is at CMB2 Dependency with Composer).

    "autoload": {
        "files": [
            "vendor/prospress/action-scheduler/action-scheduler.php"
        ]
    },

By adding the function_exists() check, we get the same behavior (except in the edge case where we're running a PHP based tool, so WordPress is not loaded, and we don't want to init).

Another way to look at this is when running outside the context of WordPress we do not want to autoload action-scheduler.php however Composer does not allow us to add a conditional like this, so instead we add the conditional check inside action-scheduler.php.

[thenbrent]

Perfect, thanks for the explanation.

Another way to look at this is when running outside the context of WordPress we do not want to autoload action-scheduler.php however Composer does not allow us to add a conditional like this, so instead we add the conditional check inside action-scheduler.php.

I think that's the context I was looking for.

The function_exists() check really does seem like the only sane way to handle this then.

@johnbillion I really appreciate you sharing your expertise on this thread. Any final thoughts before I merge #304?

[johnbillion]

I think this is okay to change for now. I have a few ideas but they can wait and I'll open a follow-up ticket. 👍

[thenbrent]

Fixed via #304.