Scenario:

• WPSEO
• Another plugin using WHIP with the same requirements

When the requirements are not met, the notice is shown twice.

This is because, for every time whip_wp_check_versions is called (and requirements are not met), \Whip_WPMessagePresenter::register_hooks is also called.

This creates as many hooks to admin_notices as many calls to whip_wp_check_versions there are.

I think the problem is that whip_wp_check_versions creates a new instance of Whip_WPMessagePresenter every time is called.

Something like that may prevent duplicate notices:

function whip_wp_check_versions( $requirements ) {
	//...

	global $whip_message_presenter;
	if ( ! isset( $whip_message_presenter ) ) {
		$mostRecentMessage      = $checker->getMostRecentMessage();
		$whip_message_presenter = new Whip_WPMessagePresenter( $mostRecentMessage );
		$whip_message_presenter->register_hooks();
	}
}

But I'm not sure if that's the best approach.

Nightly has become PHP 8.0 since PHP 7.4 has been branched, so to continue to also test against PHP 7.4, it needs to be added separately.

Refs:

• https://twitter.com/nikita_ppv/status/1089839541828112384
• https://twitter.com/nikita_ppv/status/1094897743594770433

When the admin_notices hook is created, raise a flag in a global variable to avoid hooking multiple times.

See #36

I would like to add tests coverage to this whole class, but I'd need to add 10up/wp_mock, lucatume/function-mocker or something similar to mock global functions such as wp_kses.

If that's not a problem, I can add tests to this PR in a separate commit.

Use the new property array format. The old format is still supported for now, but will be removed in PHPCS 4.0.

See: https://github.com/squizlabs/PHP_CodeSniffer/releases/tag/3.3.0

As this repo does not have a composer.lock file, no other changes are needed AFAICS.

Once PHP 7.3-beta came out, the nightly build on Travis became PHP 7.4-dev and builds haven't been tested against PHP 7.3 for months now.

As of this week, Travis has (finally) made a PHP 7.3 alias available now RC3 is out, so I've ~~updated the highest PHP version to test against to~~ added a build to test against PHP 7.3 so there won't be any unwelcome surprises when PHP 7.3 comes out.

Bug description:

• The parameter order for most calls to this Exception was incorrect.
• Where it was correct, a gettype() was used, while this is already done in the exception class itself, leading to incorrect results.

PR notes:

• Includes improvement to the parameter documentation of the Exception constructor.
• Includes unit tests proving the bug and safeguarding against it in the future.

CS: fix regex layout

Using the x modifier allows for using comments within a regex itself removing the need for the concatenation previously used. The x modifier ignores all whitespace within the regex, allowing for a nice layout as well. To facilitate this, I've moved the regex delimiters up into the regex string, so the modifier and the regex stay together.

Refs:

• http://php.net/manual/en/reference.pcre.pattern.modifiers.php

CS: minor regex efficiency improvement

No need to remember sub-matches which are not being used anyway (previously match 2 and 3). The parentheses were in this case also not needed for grouping, so removing them was the simplest fix.

Testing this

There are already unit tests in place which cover this method comprehensively, so if I did things right, those should still pass. Sic: and they pass as expected

The class and interface names and how they relate to each other are not really clear and can be misleading without reading their source.

(I'll omit the prefix/pseudo-namespace for brevity.)

Right now:

VersionDetector => detects version AND provides message VersionMessage => sets criteria AND acts as controller MessagePresenter => renders message AND disentangles conflicting Whip versions

Proposed layout:

(off the top of my head, might need adjustments)

Message => value object representing a message VersionRequirement => value object representing a "component => minimum version" association VersionTester => gets a VersionRequirement and returns whether it is met ComponentMessage => gets a failed VersionRequirement and turns it into a renderable Message MessageFilter => filters the collection of Message objects to only show the relevant ones (to disentangle conflicting Whip versions) MessagePresenter => renders all Message objects RequirementsCheck => 1. runs each passed-in VersionRequirement through the matching VersionTester, 2. fetches the matching ComponentMessage for each failed VersionRequirement, 3. passes all collected Message objects through the MessageFilter, 4. Passes the filtered collection of Message objects to the MessagePresenter. CheckVersions => facade to provide simplified access to execute a version check.

Proposed normal flow:

(I added MySQL as well to demonstrate usage with multiple requirements.)

// Hook into user interface.
$message_presenter = new WPMessagePresenter();
$message_presenter->register();

// Set up requirements.
$requirements = new RequirementsCheck(
    $message_presenter,
    [
        new VersionRequirement( 'php', '>=5.6' ),
        new VersionRequirement( 'mysql', '>=5.7' )
    ]
);
$requirements->check();

Proposed simplified flow (using the Facade):

CheckVersions( [
    'php'   => '>=5.6',
    'mysql' => '>=5.7',
] );```

The message include the line: We have an article on how to test whether that's an option for you here. This is probably meant to include a link to said article.

Dynamic (non-explicitly declared) property usage is expected to be deprecated as of PHP 8.2 and will become a fatal error in PHP 9.0.

There are a number of ways to mitigate this:

• If it's an accidental typo for a declared property: fix the typo.
• For known properties: declare them on the class.
• For unknown properties: add the magic __get(), __set() et al methods to the class or let the class extend stdClass which has highly optimized versions of these magic methods build in.
• For unknown use of dynamic properties, the #[AllowDynamicProperties] attribute can be added to the class. The attribute will automatically be inherited by child classes.

In this case, both the $configuration property, as well as the $messageManager property (both set in the class constructor) fall in the "known property" category and should be declared.

Note: while this could be considered a BC-break, I've elected to make these properties private (was previously automatically public due to these being dynamic properties).

Refs:

• https://wiki.php.net/rfc/deprecate_dynamic_properties

TL;DR: The tests will now be run against PHP 8 and are currently passing.

Tests: autoload test files via Composer

The Composer autoload-dev directive allows for specifying files which should only be autoloadable in a dev environment.

By adding this directive to the composer.json file, we can get rid of most require_once statements in the test suite.

The only one remaining is the loading of the WP Core functions mocks. This could be done via a files directive within autoload-dev, but then they would always be loaded, while they are only needed in the context of running the tests.

So instead, I've added the require for the WP Core functions mock file to the test bootstrap file.

A side-effect of this change is that we can get rid of the setUpBeforeClass() methods which are problematic for PHPUnit cross-version compatibility due to the addition of the void return type to the method signature in PHPUnit 8.

Tests: add base TestCase

To make the tests cross-version compatible for the WHIP module, not that much is needed in terms of compatibility polyfills.

The most pertinent issue is to do with testing exceptions.

This commit adds a base TestCase from which all WHIP test classes will extend from now on. This TestCase class only contains a helper method to test exceptions in a PHPUnit cross-version compatible manner.

Includes adding the new testCase to the PHPCS ruleset so sniffs which behave differently in test situations will recognize these correctly.

Tests: implement use of the TestCase::expectExceptionHelper() method

.. to replace the @expectedException... annotations which have been removed in PHPUnit 9.

Tests: implement work-around for remaining PHPUnit cross-version issue

PHPUnit 9 removed the assertInternalType() method in favour of more specific methods, which were introduced in PHPUnit 8.

As this test suite only uses this assertion in one place, putting a polyfill in place is over the top. A simple if/else does the trick.

Composer: allow installation of PHPUnit 8 and 9

Now the tests have been made cross-version compatible with PHPUnit 8 and 9, allow installation of those versions by widening the version restraints in the composer.json file.

PHPUnit 8 introduces a form of caching to PHPUnit, so adding this cache file to the .gitignore file.

Travis: run the tests against PHP 8/nightly

Now the tests are fully compatible with PHPUnit 9, the test suite can be run on PHP 8.

~~As not all dependencies of PHPUnit have tagged a new version which allows for PHP 8 yet, we do need to ignore-platform-reqs for the time being.~~

Includes adding travis_retry to composer install-like commands to get round builds stalling on the connection with Packagist, especially on older PHP versions.

WIP: Still need to update tests

Context

• The built-in messaging is very specific for PHP 5.6 which is no longer relevant. Instead of having to release WHIP with a new message for every PHP version, we need to be able to provide a custom message.

Summary

This PR can be summarized in the following changelog entry:

• Add support for customized messages.

Relevant technical choices:

• BC break: the WordPress fasade now requires an array per component instead of just a version constraint. This is done to match a version requirement to message.
• BC break: I've added version and operator to the WHIP_Requirement interface. The code already relied on these functions. and in the context of WHIP every requirement is a version constraint, so I didn't think adding w WHIL_Version_Requirement. was the way to go.

In the Whip_Host.php file, there are a couple of calls to apply_filters() with variable filter names.

I'm not familiar enough with the code to properly review these, but I do believe this setup should be reviewed for the following reasons:

• Variable filter names with unpredictable values make it hard to hook into these.
• All hook names should be prefixed to prevent conflicts with other plugins/themes. This is not safeguarded in the current setup.

Relevant warnings thrown by PHPCS when run without exclusions:

FILE: src\Whip_Host.php
------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
------------------------------------------------------------------------------------------
  47 | WARNING | Hook names invoked by a theme/plugin should start with the theme/plugin
     |         | prefix. Found: "strtolower( self::HOST_NAME_KEY )".
  77 | WARNING | Hook names invoked by a theme/plugin should start with the theme/plugin
     |         | prefix. Found: "strtolower( $messageKey )".
 102 | WARNING | Hook names invoked by a theme/plugin should start with the theme/plugin
     |         | prefix. Found: "self::HOSTING_PAGE_FILTER_KEY".
------------------------------------------------------------------------------------------

Issue https://github.com/Yoast/whip/issues/40 correctly suggested to get rid of the global variable (globals are always bad and should be avoided at all cost).

However, the proposed solution in turns a single check into three function calls that are processed through major parts of the WordPress system (and, ironically, rely on multiple globals as well).

I better fix would have been to turn the global variable into a method-scoped static variable. Serves the same purpose, has no impact outside of the method and does not produce unneeded overhead.

So, something simple like this would have been preferable:

function whip_wp_check_versions( $requirements ) {
    // ...
    static $is_registered = false;
    if ( ! $is_registered ) {
        // Register!
        $is_registered = true;
    }
}

On subsequent calls, the first line is skipped at compile-time already, and the second immediately fails, skipping a duplicate registration. No globals involved (even indirectly through $wp_filters).

I'm not sure if this is where I would ask this question. I recently wrote my host with this email. "Hi, For security reasons and an increase in speed I want to update the PHP version of my WordPress website to PHP 7. WordPress also strongly recommends the latest version of PHP on their requirements page, https://wordpress.org/about/requirements/.

More information about currently supported versions of PHP can be found here: http://php.net/supported-versions.php.

I've heard that sometimes a website can break when updating the PHP version. Can you assist me and update my site safely to PHP 7?

Thanks!"

They responded back, but I'm totally confused! Could someone advise me on how to handle this because I'm really not understanding all the terms and lingo. Thanks so much!

Oh here was their response: "Upon checking your requirement, I could see that the you want to upgrade the PHP 7 for a domain. It can be possible only if we can upgrade easyapache4. EasyApache 4 supports multiple versions of PHP. Multiple PHP versions allow you to assign different PHP versions to each of your domains. Coupled with automatic upgrades, this ensures that your PHP applications run on the most up-to-date, secured PHP versions.

However on checking I could see that the server is running on CentOS 6. We always recommend you to use EasyApache4 on new CentOS version which is 7, as upgrading EasyApache4 in CentOS 6 may break the working of sites. If you wish we can migrate your server from CentOS 6 to CentOS 7 for free of cost. If you would like to proceed with Migration, then please let us know so that we shall move the ticket to our Migration team.

Please contact your developer to check the compatibility of the application with new PHP version before upgrading.

However, please note that any apache customizations present in the server will get lost. Also the compilation process will cause a tleat 10 minutes of downtime. So it is better to do the process on off peak hours."

Technically by being 'removable' once the issue is resolved (upgraded PHP) this is fine per the plugin guidelines. That's not this. This is a thought about keeping the WP Dashboard usable.

Seeing as this is not something that everyone can fix in one click, and its very LARGE (which IMO it should be), it might be prudent to be less invasive on global usability. My thoughts are to allow it to be dismissible but:

1. Keep an alert in the WP Admin Bar
2. Keep a number on the upgrade menu (and show the message on the upgrade panel)
3. Keep a one-line alert "Upgrade PHP!" which expands when clicked
4. Keep a BRIGHT RED tab by options/support with the information

Mind, WP needs a notification control center overall but that's a different horse to beat.

I'm spitballing here, since there needs to be a balance between UPDATE PHP!!!! and making sure it's not ignored.

The interface Whip_Requirement wants to be an abstract requirement that only knows about a ::component(). However, in Whip_RequirementsChecker::requirementIsFulfilled(), the requirements version() is being checked, despite the interface not having such a method.

So, either Whip_Requirement is actually a Whip_VersionRequirement, or the Whip_RequirementsChecker::requirementIsFulfilled() needs to abstract away what it actually checks.

To abstract this away, it can delegate the isFulfilled() to the requirement itself with all related knowledge it has.