[4.3] Fix issue Deprecated: trim(): Passing null on PHP 8.1

[sonvnn]

Pull Request for Issue #38510 .

Summary of Changes

If $path is null, it will get '' value instead of run trim() function.

Testing Instructions

1. Open templates/cassiopeia/index.php
2. Add code below at the line 17:

use Joomla\CMS\Filesystem\Path;
echo Path::clean(null);

3. Switch template to cassiopeia, go to front-end and check it.

Actual result BEFORE applying this Pull Request

Deprecated: trim(): Passing null to parameter #1 ($string) of type string is deprecated in /{root}/libraries/src/Filesystem/Path.php on line 198

Expected result AFTER applying this Pull Request

Warning disappear

Documentation Changes Required

No

[wilsonge]

Think we need some more info here because the is_string call immediately above this line should fail before you've reached the trim

[zero-24]

I agree I would say we should not pass an empty string to that method in the first place.

[sonvnn]

Think we need some more info here because the is_string call immediately above this line should fail before you've reached the trim

@wilsonge @zero-24

If $path = NULL it will pass if (!\is_string($path) && !empty($path)) {

I test in my local with var_dump
var_dump($path, is_string($path), empty($path));

Result Below

[zero-24]

Yes thats the reason I would not like to add that if else catch in every method that is handeling strings and hide the issues of passing null or empty strings to methods which should not be called with empty strings in the first place.

[sonvnn]

Yes thats the reason I would not like to add that if else catch in every method that is handeling strings and hide the issues of passing null or empty strings to methods which should not be called with empty strings in the first place.

Yeah! I fixed it in my component. Anyway, I think other extensions will face that when customer update PHP to 8.1

[zero-24]

yes when the extension is ready for 8.1 that message does not come up right? When it is not ready for 8.1 yet the issue should be reported to the developer so it can be fixed right? Right now its not an fatal error by php yet anyway so "hiding" error messages "hides" the issue in the same way but allows the developer to find and fix it too.

[richard67]

Maybe the
if (!\is_string($path) && !empty($path)) {
should be changed to something like
if ($path === null || (!\is_string($path) && !empty($path))) {
?

[laoneo]

As long as a library accepts null values is should work properly and not throw different errors on different PHP versions. So making a change in the library is fine.

[sonvnn]

Maybe the if (!\is_string($path) && !empty($path)) { should be changed to something like if ($path === null || (!\is_string($path) && !empty($path))) { ?

You are right! I updated code with if ($path === null || (!\is_string($path) && !empty($path))) {

Best Regards,

[laoneo]

This is now a BC break as an exception is thrown when the path is null, which wasn't before.

[sonvnn]

This is now a BC break as an exception is thrown when the path is null, which wasn't before.

May you me give a suggestion? Raise an exception to force developer fix in their code or set $path = '' to ignore it. I'm wondering about that.

Thanks & Best Regards,

[laoneo]

I would go with something like:

if ($path === null) {
	@trigger_error(
		sprintf(
			'Path can not be null, in 5.0 it will throw an exception',
			__METHOD__
		),
		E_USER_DEPRECATED
	);
	$path = '';
}

[sonvnn]

@laoneo Thank you for the suggestion! Because no one came up with a better idea. I have updated the code as per your suggestion.

Please help me check it again!

[laoneo]

Looks correct now. Thanks!

[laoneo]

When you update the test instructions and expected/actual result the way, that the testers know what to do, then it is very likely that you change gets merged. You can put something like in testing instructions with some files in core:

• Add this code to the file XY at line ZZ
• Open the page XY

Similar to what I did in #38535.

[sonvnn]

@laoneo Thank you! I updated test instructions.

Best Regards,
Sonny

[alikon]

I have tested this item ✅ successfully on 3ae8337

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38511.}

[jwaisner]

I have tested this item ✅ successfully on 7e61399

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38511.}

[jwaisner]

@alikon , can you please retest since a change was made?

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38511.}

[alikon]

no need to retest @jwaisner only branch merge

[alikon]

I have tested this item ✅ successfully on daaf175

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38511.}

[alikon]

RTC

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38511.}

[laoneo]

This has to go into 4.3 as it adds a deprecation. So waiting for the upmerge to make a rebase.

[laoneo]

PR for manual can be found here joomla/Manual#33.

[obuisard]

Thank you all for working on this issue!