fix: initialize typed properties correctly in ExternalImage::build()

[jrathert]

Related:

• Ticket [BUG] Accessing alt on ExternalImage throws error #2817

Issue

The (protected) typed properties of ExternalImage, $alt_text and $caption, were not initialized properly in the static build() method, causing errors when accessing them later (must not be accessed before initialization). The error only occurred in PHP >= 7.4, when this behavior (not initializing typed properties) was introduced.

Solution

The calling method (Timber::get_external_image()) provides empty strings as default arguments for alt and caption when calling the build() method. Also, within the build method, there was a default for at least the alt parameter. But as these default parameters were empty strings, they were not used to initialize the properties properly. I did two things:

1. Ensure that also caption has a default value (empty string) defined within build()
2. Initialize properties even if the parameters are empty strings.

Impact

No negative impact expected, as typed properties did not even exist before PHP 7.4, so that version is required for Timber 2.0.

Usage Changes

None

Considerations

One could argue that null values would be even more appropriate as default initialization values for the respective properties, if they are not specifically defined by the user. That would allow to separate between "there is no alt value" and "there is an alt value, the empty string".

Testing

I enhanced the unit test for the ExternalImage class to cover the case where a user does NOT specify an alt-text or a caption when creating the ExternalImage with Timber::get_external-image(). These test cases fail for the original code but succeed when the patch is applied.

[gchtr]

Thanks for the pull request @jrathert!

I’d like to get @nlemoine’s opinion on this: Should we go for empty strings or maybe work with a nullable string property?

protected ?string $alt_text = null;

[jrathert]

protected ?string $alt_text = null;

If we do it like that (probably a better approach, as I mentioned in the considerations), then we

1. should do it for caption as well
2. need to change the definition of the corresponding functions to reflect the possibility of a null value (public alt(): string|null, same for caption())
3. must reflect the latter in the interface ImageInterface, otherwise they won't match
4. need to do the same for Attachement::caption() and in Image::alt()

This is why I propose not to initialize with null. The value of it would also be a bit limited - in reality, people would most likely always do {{ img.alt|default('') }}, if we allow for a null-value. So an empty string is enough to have, IMHO.

[nlemoine]

@jrathert Thanks for fixing this!

I agree with your approach. The only thing that I would so differently is not initializing the properties but rather use the null null coalescing operator when returning values.

You can see it right here : #2825

Sorry to push another PR, It was faster than commenting code and I didn't find an easy way to PR on your fork 🙂 Feel free to report changes if needed.

[jrathert]

@nlemoine - no problem tweaking my patch into a separate PR, but:

The only thing that I would do differently is not initializing the properties but rather use the null null coalescing operator when returning values.

I am strongly against this. Not only it is a bit a questionable approach: first we make sure (in some places) by default arrays to set the values to empty strings - just to in the very last moment of access replace that empty string with null? Also, the return value of the class method (?string) no more exactly matches the interface (string). And users will most likely anyway always do: img.alt|default(''). All things I mentioned above.

But the most important and critical thing with your approach now: What if a user explicitly sets alt to the empty string '' (via ExternalImage:set_alt()) - and we then return null when accessing it? That is at least confusing.

So either we do it right from the very beginning (allow null properties and initialize them that way and only return null if not explicitly changed) or do not return null at all (and keep it the way I proposed).

[nlemoine]

first we make sure (in some places) by default arrays to set the values to empty strings

Are you referring to wp_parse_args that sets default values? Strictly useless if we maintain these checks:

timber/src/ExternalImage.php

Lines 150 to 156 in 38a9940

	if (!empty($args['alt'])) {
	$external_image->alt_text = (string) $args['alt'];
	}
	if (!empty($args['caption'])) {
	$external_image->caption = (string) $args['caption'];
	}

no more exactly matches the interface

This was changed in my PR to string|null and makes more sense because we can tell if the property was set or not (which we can't tell if it's always an empty string).

And users will most likely anyway always do

We should avoid making assumptions on what users might do, it's WordPress 😉

What if a user explicitly sets alt to the empty string '' (via ExternalImage:set_alt()) - and we then return null when accessing it?

I think you're mistaken by the null coalescing operator. If a user sets an empty string, $image->alt() will return an empty string.

Depending on what you're doing, you don't necessarily need to initialize your properties.

[jrathert]

@nlemoine - sorry, my bad, I made a few stupid mistakes here. Wasn't the best idea to comment while commuting and not having convenient access to the code. And to top even that: I really wasn't sure about the null coalescing operator's behavior and tried it before commenting, but the test I did was simply not correct / misleading.... 😬

So fine with it, sorry again.

[gchtr]

Thanks to you both for figuring out the correct path to this. I just approved the PR in #2825.

@nlemoine I think we’ll wait until we fixed all the testing bugs before merging this in.

Sorry to push another PR, It was faster than commenting code and I didn't find an easy way to PR on your fork 🙂 Feel free to report changes if needed.

@nlemoine We usually do it this way: create a new PR based on the commits of the contributor and make the PR not against the PR, but against the main branch. This worked fine in the past.

sorry, my bad, I made a few stupid mistakes here. Wasn't the best idea to comment while commuting and not having convenient access to the code. And to top even that: I really wasn't sure about the null coalescing operator's behavior and tried it before commenting, but the test I did was simply not correct / misleading.... 😬

@jrathert No worries, and don’t call them stupid mistakes. That’s why we have PRs and discussions. I also learn like a looot of new things almost every time I try to tackle an issue. It’s amazing that you already dived deep into the project even though you don’t have that much experience with PHP. Your help is invaluable and we truly appreciate it 🙌. Thanks!

[gchtr]

Just an additional note: as soon as we merge #2825, this PR will be marked as merged automatically.

[nlemoine]

No need to be sorry @jrathert ;) Thanks for your contribution!