Uncaught assertions causing fatal errors on PHP 8+

[nmolham-godaddy]

Summary

Prevent assert debug statements from throwing fatal errors.

Story: MWC-11044

Release: #705

Details

Added a SV_WC_Plugin::assert method to perform the assertion and log the failed ones without causing fatal errors.

QA

• Review and merge PSR4 & Class Map Autoloading #704
• Code review

Before merge

• PSR4 & Class Map Autoloading #704 is merged
• Make sure to merge this PR into Release v5.14.0 #705 branch
• I have confirmed these changes in each supported minor WooCommerce version

[nmolham-godaddy]

@agibson-godaddy you are much better than me in Github actions, I have been trying for a while getting this working but I kept failing. The idea is configure the ini setting zend.assertions=1 for the new test to pass.
On my local env, I have that configured and works as expected, but I am having a very difficult time making it work online.

Can you take a look?

[agibson-godaddy]

Code wise this LGTM! Nice work.

I do wonder if separately we need to re-evaluate the usefulness of these assertions? For example:

public function get_gateway_class_name( $gateway_id ) {

	$this->assert( isset( $this->gateways[ $gateway_id ]['gateway_class_name'] ) );

	return $this->gateways[ $gateway_id ]['gateway_class_name'];
}

That's meant to be validating the gateway class name isset. But if it's not set then we'll actually still end up here:

return $this->gateways[ $gateway_id ]['gateway_class_name'];

and then it will trigger a PHP notice if that index doesn't exist. Point being: even if the assertion fails, we're still continuing with our code anyway, which seems counter-intuitive.

To be clear: I don't think this is a problem with your changes, which is why I'm still approving this. It's a more broad comment on our use of assert() in general. Maybe some of them make sense, but the one I pasted above is an example that just seems a bit odd.

[nmolham-godaddy]

Thanks @agibson-godaddy for solving that ini config problem, impressive work as usual.

Regarding your comment about the assert usage: Now as I think of think based on your comment, I agree that some of them are counter-intuitive indeed 🤔

I assume the main purpose of these assertions are making sure the necessary data structure for the gateways to function are defined in the implementation of these classes.

I will open another PR to clean them up and validate each one use-case, but for now, I will merge this PR to release the new framework version 🥳