Fatal error when gathering postmeta when deleting a donation

[decodekult]

Hi there!

I got redirected here from a support thread in wordpress.org

We have a plugin that stores some metadata on posts when a snippet from us is used on their post content. We need to do some cleanup when those posts are deleted. As this snippet can be placed in any post type, we hook into the WP before_delete_post action to check whether the post meta exists and eventually run our cleanup. But this is throwing a PHP fatal error on clients using Give WP, and they headed to us for support. I checked the problem and I do not think it is on us.

Steps to reproduce:

• Install Give WP, and configure a test form to generate test donations. Generate a couple of donations.
• Place the following snippet as a MU plugin on your site:

add_action( 'before_delete_post', function( $post_id ) {
	// See https://developer.wordpress.org/reference/functions/get_post_meta/
	// get_post_meta should be returning an ARRAY
	// regardless of whether there is a meta entry for this post and this meta key
	$meta = get_post_meta( $post_id, 'toolset-meta' );
	if ( count( $meta ) > 0 ) {
		// Clean our internal cache
	}
});

• Head to the site backend and delete one of the donations.

A fatal error will be thrown: PHP Fatal error: Uncaught TypeError: count(): Argument #1 ($value) must be of type Countable|array, string given. My PHP version on this test site is 7.4, by the way.

---

After some debugging, I narrowed this down to Give_DB_Meta::get_meta. At some point, and regardless of the arguments passed to the get_post_metadata filter that this method is hooked into, the method decides that it should return a string:

public function get_meta( $id = 0, $meta_key = '', $single = false ) {
	if ( ! $this->is_filter_callback ) {
		return get_metadata( $this->meta_type, $id, $meta_key, $single );
	}

	$id = $this->sanitize_id( $id );

	// Bailout.
	if ( ! $this->is_valid_post_type( $id ) ) {
		return $this->check;
	}

	if ( $this->raw_result ) {
                // Here, this method returns a string (hence the FALSE parameter to get_metadata, and a default value of an empty string) regardless of the $single value passed to the method.
		if ( ! ( $value = get_metadata( $this->meta_type, $id, $meta_key, false ) ) ) {
			$value = '';
		}

		// Reset flag.
		$this->raw_result = false;

	} else {
		$value = get_metadata( $this->meta_type, $id, $meta_key, $single );
	}

	$this->is_filter_callback = false;

	return $value;
}

I would like to understand the logic behind this decision, to better understand if there is any good reason for us to go on and patch this on our side. In my opinion, we should not be providing a patch for our clients since we are legitimately using a WP API hook expecting it to return what the official documentation states it should return - in other words, I would kindly ask you to review this piece of logic and adjust it so it fits the documented behavior of the filter it is hooking into.

Thanks in advance.

[kjohnson]

It looks like that line was a part of the original commit for that class back in August of, 2017. I don't see any additional commit history or documentation to explain why this was written to return a string in that instance.

As for the error, this was written back in the 5.6/7.0 days when this would have been a warning (up until 8.0, for which we are still finalizing support).

That said, we should be respecting the return types of WP API hooks and I'll bring this to the team so that we can incorporate this into our work on 8.0 compatibility.

[kjohnson]

@decodekult in the meantime, since WordPress does not enforce return types I would suggest adding a is_countable check as defensive programming.

if ( is_countable($meta) && count( $meta ) > 0 ) {
    // Clean our internal cache
}

[JasonTheAdams]

Hi @decodekult!

As you can imagine, that code is rather deeply engrained in a rather old part of GiveWP, and the way it works — modifying post meta calls — makes it difficult to track down. I took a stab at cleaning it up to see if it would cause absolute chaos. The good news was that it didn't seem to rain to fire, but I did find at least one part of our forms that break. This tells me at least some parts of the GiveWP code rely on this (admittedly odd) behavior.

The other good news is that this portion of our codebase is becoming increasingly legacy. Soon we'll be releasing GiveWP 3.0 which introduces an entirely new kind of forms that do not rely on this legacy code. We'll still be supporting the older (called v2) forms for a while, but moving everyone towards the new (v3) forms as quickly as possible. We then plan on cleaning up a lot of the legacy code in 4.0.

So where does that put us now? Well, the reality is that this meta issue is probably not going to be resolved in the near future. The infrastructure doesn't allow us to safely change its behavior without risking the integrity of our customers' donation forms, which we take seriously. We may give it another shot later, but I don't want anyone to wait on that.

In the meantime, I believe @kjohnson's proposed solution is the best path forward that shouldn't cause any adverse side-effects for your plugin. Since the appropriate behavior of the meta is to be countable, it's merely a minor redundancy. It's simply the fastest, and safest way (on both sides) to resolve the compatibility issues.

Thank you so much for bringing this to our attention. We always want to wisely and responsibly make GiveWP better with the best interest of the echosystem and our customers in mind.

[decodekult]

Hi there, and thanks for the update.

I am sorry, but I have to disagree about the proposed solution. I find the issue really odd, and I agree with you about that, but I find it also quite odd that I should need to review my plugin (together with all my other plugins, which are quite a number on the company I work for), scan for get_post_meta and check every time whether the returned type matches what the docs state it should return.

It might be a merely minor redundancy if my plugin were using an obscure WordPress function, and just once. This is a quite well-used and established function that we alone use dozens of times, mind about other plugins. In fact, I am quite surprised that this was not raised to you guys earlier.

This kind of "defensive programming" is a can that never gets to its bottom. I do not find it defensive, but unreliable: we can not account for all third parties doing this kind of thing, and prevent issues caused by their "odd" behaviors. Should we also check any other WordPress core function against its returned type, regardless of what the official docs state? Should we get to the core codebase and "get safe" on any functions with a filtered return value?

I am sorry, but I understand that instead of patching the unexpected returned type in your limited codebase, you are expecting that others patch their legitimate usage of a legitimate core function with proper return types in their whole, eventually larger, codebases. I find it... odd.

Thanks for pointing out that you have plans to offer alternatives to this codebase on a future v3 release, and a cleanup on a future v4 release. For the record, we have at least one shared client who is quite unhappy with the current state of this issue, and the need of patching plugins on every new release, but I do not think that we can do anything reasonable to ease that situation on our side.

[JasonTheAdams]

Hi @decodekult!

I appreciate you taking the time to reply and sharing your side of the fence. Obviously, it's challenging to appreciate the extent of the effort on either side, as we're only intimately familiar with our own codebases. We very much respect the complications on your side.

It sounds like it's not a simple or reliable solution on your end. I falsely assumed that the issue in question was happening in a single place in your codebase. It sounds like it's far more widespread than that. If it was one place, then adding an otherwise redundant check wouldn't to be too bad (albeit, not pleasant), but if it's all over the place, I'd agree it's a far less plausible option.

Please understand I'm fully aware that what GiveWP is doing to the meta is odd. We completely own that. This bug is entirely on us. My goal in engaging was to see if we could arrive at a simple and small fix on either side. It's unfortunate that this doesn't seem feasible, as at this time we're not able to reliably correct the strange behavior without causing significant risk.

I'm going to internally elevate this to a higher priority post 3.0 launch. GiveWP is effectively breaking default WordPress behavior, and I can't imagine your plugin is the only one having an issue.

Thanks so much for discussing and considering this with us.

[JasonTheAdams]

Hi @decodekult!

I just wanted to circle back and let you know we have a PR that was just approved and is now being sent to QA for testing: #7062

Hopefully that will go through smoothly and we'll get a release out in the next week or two with this in place!