Brain-WP/BrainMonkey

Prepare for PHP 8.4

Closed this issue · 11 comments

jrfnl commented

I've just manually triggered a test run and the PHP 8.4 build is currently failing for a variety of reasons.

  1. Patchwork is not ready. I've opened an issue in the repo to discuss the solution direction for Patchwork.
  2. BrainMonkey is affected by the PHP 8.4 deprecation of implicitly nullable parameters. PR to fix this upcoming. See #147
  3. BrainMonkey is affected by the PHP 8.4 deprecation of passing E_USER_ERROR to trigger_error(). This needs investigation. See #149
gmazzap commented

Thank you @jrfnl!

For point 3, I can give you more info.

TL;DR: We should probably use exceptions.

--

Patchwork can only replace functions that exist.

So when we use Brain Monkey to replace a function that doesn't exist, Brain Monkey first declares a dummy function that only triggers an error.

This means that the test will fail if the code under test executes the function without the developer setting expectations for it.

On every test, in tearDown, the Patchwork's replacement is reset, so the function goes back to being the one that triggers the error.

That means we have two possible cases in the next tests:

  • the code under tests doesn't use the function, and nothing bad happens
  • the code under tests uses the function. In that case, developers are forced to define expectations for it

Given this workflow, replacing trigger_error() with an exception will normally have the same result of making the test fail.

It currently uses trigger_error() to avoid having a too-generic expectation that "swallows" the Bran Monkey exception.

For example, image a test code like this:

public function testThatSomethingThrows()
{
   $this->expectException(Exception::class);

   $something = new Something();
   $something->execute();
}

In this case, if $something->execute() ends up throwing an exception because it calls a function for which there's no expectation set in Brain Monkey, right now, the test would fail because of trigger_error(), but if we move to use an exception, the test will pass just not for the reason the developers expect.

In summary, we have two options:

  • Keep using trigger_error() with another error constant. The risk here is that the test framework error handler would swallow such errors.
  • Move to exception, and the risk is the one described above

I think the latter is the lesser risk, so maybe we go for that.

jrfnl commented

Thanks for that explanation @gmazzap!

So when we use Brain Monkey to replace a function that doesn't exist, Brain Monkey first declares a dummy function that only triggers an error.

I've found the definition now (in a heredoc, which is why I didn't find it initially via (bleeding-edge) PHPCompatibility).

In summary, we have two options:

  • Keep using trigger_error() with another error constant. The risk here is that the test framework error handler would swallow such errors.
  • Move to exception, and the risk is the one described above

I think the latter is the lesser risk, so maybe we go for that.

I agree the second option is best, but would like to throw a variant of that in the mix:
What about adding a new Brain\Monkey\Expectation\Exception\UndefinedFunction extends Error exception ? While this may not be 100% semantically correct as Error is intended for PHP internal errors, it would reduce the chances of the exception unintentionally being caught by tests.

The only "problem" is that Error is a PHP 7.0 class, so this would need a dual definition with the class extending Exception in PHP 5.6.

This could be defined like the below and would then still be PSR4 compliant.

namespace Brain\Monkey\Expectation\Exception;

if (PHP_VERSION_ID >= 70000) {
    class UndefinedFunction extends \Error {}
} else {
    class UndefinedFunction extends \Exception {}
}
jrfnl commented

P.S.: the name of the new exception is, of course, open for discussion, my comment is mostly about the principle of how to handle this.

gmazzap commented

Hi @jrfnl and thanks for you input.

I'm unsure about extending Error. I would be fine on PHP 7+, but the conditional declaration looks complex and hard to justify.

  • If the risk model is people using too-generic expectations for generic, people can do $this->expectException(Throwable::class). Yes, this is even less expected than an expectation on Exception, but still, we do not eliminate all the risks.
  • What about running tests in a matrix? The same tests for the same codebase could sometimes be tested with Brain Monkey declaring the exception extending Error and sometimes Exception, which could cause "funny" inconsistencies.

So, all considered, I think the problems outweigh the benefits.

I guess that having some MissingFunctionExpectations extends Exception should be fine. We accept the low risk but have less complexity and consistency in matrix-based tests.

jrfnl commented

@gmazzap In that case, PR #149 should fix that part of the BrainMonkey PHP 8.4 compatibility issues.

So now it is just a waiting game for a new release of Patchwork. I've got a prelim patch for that ready, just waiting for the go-ahead on the PHP < 7.1 version drop.

  • 👍1
  • 🚀1
jrfnl commented

And we're ready! 🎉

Patchwork just released version 2.2.0, which adds runtime support for PHP 8.4. V 2.2.0 does have a minimum PHP version of 7.1, but that shouldn't be an issue. When people install via Composer and are running on PHP < 7.1, Composer will just give them the last 2.1.x version, which will work fine on PHP < 7.1. And when people install via Composer with PHP 7.1+ (including PHP 8.4), they will get v 2.2.0 (or a later release once available) and that should take care of the PHP 8.4 compat issues.

@gmazzap I'd recommend tagging a new BrainMonkey release over the next few days to get the BrainMonkey specific PHP 8.4 patches out into the world. If people then update with --with-dependencies, they should automatically also get the new Patchwork release if they are running on PHP 7.1 or higher.

jrfnl commented

@gmazzap Ping ?

gmazzap commented

@jrfnl Sorry haven't feel well. Just to confirm, there's nothing else to do here just tag, right?

jrfnl commented

@gmazzap Nothing based on the last time I checked, though that is a few weeks back. Might be good to trigger a test run against the current PHP 8.4 just to verify, but I expect we should be good. I won't have time to double-check until the weekend.

  • 👍1
jrfnl commented

Double-checked & confirmed.

As far as I'm concerned, nothing blocking the release anymore.