Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed strip_tags(): Passing null to parameter #1 in Fulltext.php #3655

Merged
merged 2 commits into from
Nov 16, 2023

Conversation

kiatng
Copy link
Collaborator

@kiatng kiatng commented Nov 14, 2023

Description (*)

In the process of updating a couple of sites to PHP8.1. This was logged:

    [type] => 8192:E_DEPRECATED
    [message] => strip_tags(): Passing null to parameter #1 ($string) of type string is deprecated
    [file] => .../app/code/core/Mage/CatalogSearch/Model/Resource/Fulltext.php
    [line] => 764
    [uri] => /index.php/admin/catalog_product/save/back/edit/tab/product_info_tabs_group_30/store/0/id/13641/key/debdde05d306ac8c1ac92215807e895d/

@github-actions github-actions bot added the Component: CatalogSearch Relates to Mage_CatalogSearch label Nov 14, 2023
Copy link
Contributor

@Sdfendor Sdfendor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this change solves the deprecation error it also changes the result of the code.
See this following example:
image

Currently in PHP 8.1/8.2 the deprecation is thrown but the built-in methods behave as before, so the result of strip_tags(null) is empty string, not null. Subsequently in cases where $value is null in line 764 it currently becomes empty string after this line is executed. With your change $value stays null when returning in line 768.

For BC sake and to avoid unforeseen consequences, I would therefore propose to set $value to empty string in the else branch of the condition:

if ($value !== null) {
	$value = preg_replace("#\s+#siu", ' ', trim(strip_tags($value)));
} else {
    $value = '';
}

@elidrissidev elidrissidev added the PHP 8.1 Related to PHP 8.1 label Nov 15, 2023
@elidrissidev elidrissidev merged commit b64754d into OpenMage:main Nov 16, 2023
17 checks passed
@kiatng kiatng deleted the strip_tags_null_deprecation branch November 16, 2023 11:22
@sreichel
Copy link
Contributor

sreichel commented Nov 28, 2023

@kiatng i'd change it a bit to guarantee work with strict types turned on

https://onlinephp.io?s=PU_tCsIwDPy9Qt8hOKEbDnF__cAHUSmhxjnoZmlaQcR3t9ucEEhyyd0l-6O7OynyKxmLngoOvjVBh5cjPtTlTgoplk-0kRgOcOqjtRUEH6mCG1pOqU6x3lSganUZ128PT2juUMw8ZJjKEt5SZPnUJLmWNRtMvsU8P8J4Qd-Uv6UtKJVUsz_HeWq0J2fRULHIz7zKuY2L5A9quKztRgWnAzY865bDI9kTvb7Gzs1owj5SfAE%2C&v=8.2.12%2C8.1.25%2C8.0.30%2C7.4.33

simple: cast to string

	$value = preg_replace("#\s+#siu", ' ', trim(strip_tags((string)$value)));

or more strict:

	$value = is_scalar($value) ? (string)$value : '';
	if ($value !== '') {
		$value = preg_replace("#\s+#siu", ' ', trim(strip_tags($value)));
	}

or:

	if (!is_scalar($value)) {
		# throw some invalid value exeption
	}

	$value = (string)$value;
	if ($value !== '') {
		$value = preg_replace("#\s+#siu", ' ', trim(strip_tags($value)));
	}

@kiatng
Copy link
Collaborator Author

kiatng commented Nov 28, 2023

From googling, the idea of declaring strict typing is to get PHP to throw errors early on so that bugs can be avoided. I get it if it's a new class that we are coding. But for existing classes, doing something like

$value = preg_replace("#\s+#siu", ' ', trim(strip_tags((string)$value)));

seems to me, defeats the purpose of catching the bugs early on. Strict typing is bypassed, the directive is ignored. So, I would argue that this is better:

        $value = $value === null ? '' : preg_replace("#\s+#siu", ' ', trim(strip_tags($value)));

Let PHP throws the error when we have this declare(strict_types=1).

Taking a step back, is there a compelling reason to insert declare(strict_types=1) on existing classes?

@sreichel
Copy link
Contributor

I dont like casting to (someting) too, thats why i'd prefer the more strict one.

In case of scalar value casting is pretty safe.

@sreichel
Copy link
Contributor

sreichel commented Nov 28, 2023

Taking a step back, is there a compelling reason to insert declare(strict_types=1) on existing classes?

  • Without php does that type casting internally - and with a later version maybe it is deprecated too.

  • Strict types ensure to pass correct type to internal php methods and also validates @method annotations !!!

    @method $this doSomething(<validates-type> $value)
    

I'm working on this, b/c it seems to be a large benefit for better code - even if we do not have typed parameters/returns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: CatalogSearch Relates to Mage_CatalogSearch PHP 8.1 Related to PHP 8.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants