joomla / joomla-cms

Home of the Joomla! Content Management System
https://www.joomla.org
GNU General Public License v2.0
4.73k stars 3.64k forks source link

[5.0] Deprecated message: libraries/src/HTML/Helpers/Select.php #41783

Open Ruud68 opened 12 months ago

Ruud68 commented 12 months ago

Steps to reproduce the issue

create a filter field like below that will NOT return a (in this case) item:

        <field
            name="created_by"
            type="sql"
            sql_select="a.id, a.created_by, u.id, u.name AS name"
            sql_from="#__example_clickedlinks AS a"
            sql_join="#__users AS u ON a.created_by = u.id"
            sql_group="a.created_by"
            sql_order="u.name ASC"
            key_field="created_by"
            value_field="name"
            onchange="this.form.submit();"
            >
            <option value="">COM_EXAMPLE_CLICKEDBY_FILTER</option>
        </field>

Expected result

empty filter without errors

Actual result

Deprecated: trim(): Passing null to parameter # 1 ($string) of type string is deprecated in /var/www/html/test/50/libraries/src/HTML/Helpers/Select.php on line 388 and Deprecated: explode(): Passing null to parameter #2 ($string) of type string is deprecated in /var/www/html/test/50/libraries/src/HTML/Helpers/Select.php on line 557

Additional comments

change line 388 in libraries/src/HTML/Helpers/Select.php from:

$obj->{$options['option.text']} = trim($text) ? $text : $value;

to

$obj->{$options['option.text']} = trim((string) $text) ? $text : $value;

change line 557 in libraries/src/HTML/Helpers/Select.php from:

$splitText = explode(' - ', $text, 2)

to


$splitText = explode(' - ', (string) $text, 2)
Ruud68 commented 11 months ago

more examples: https://issues.joomla.org/tracker/joomla-cms/41803#event-756087


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41783.

brianteeman commented 7 months ago

Unable to replicate with php 8.1.10

perhaps this has been solved elsewhere?

Quy commented 7 months ago

Closing as non-reproducible.

Ruud68 commented 7 months ago

Code has not been changed (as debugged above where the root cause is). so here you go again with php 8.2.15 in this case image

Quy commented 7 months ago

Please advise exact steps in which file as I cannot reproduce it. Which Joomla version? Thanks.

Ruud68 commented 7 months ago

Latest Joomla 5 version.

I have done a root cause analysis and proposed a fix when you do a code review for this it should be clear:

Trim requires a string as input. The result of the sql field is mixed: string when a value is goud or null when there is no value found. The null (empty sql result) is then passed to the trim function that will then throw this error.

No need to replicate

Question here is how do we handle 'user' input? In this case we trust the sql field to always return a value.... Which it doesn't

So we either force the input to be a string via casting (that is what I did in proposed solution), or do not accept other value then strings.

There are more places where these situations can occur in core code: assumptions being made of the received input being correct

I have already linked to an example in previous comment.

alikon commented 7 months ago

unable to replicate this query have 0 results

<field
    name="created_by"
    type="sql"
    sql_select="a.id, a.created_by, u.id, u.name AS name"
    sql_from="#__content AS a"
    sql_join="#__users AS u ON a.created_by = u.id"
    sql_where="u.id = 0"
    sql_order="u.name ASC"
    key_field="created_by"
    value_field="name"
    onchange="this.form.submit();"
    >
    <option value="">COM_EXAMPLE_CLICKEDBY_FILTER</option>
</field>
Fedik commented 7 months ago

@alikon it is PHP issue, when there is a call of select.option with null instead of expected string, like:

HTMLHelper::_('select.option', 'foobar', null);

Not that it is correct call, but it is what it is

Ruud68 commented 7 months ago

Did some more debugging, issue arises when SQL field holds value NULL.

in administrator/components/com_content/forms/filter_articles.xml

add the following filter field:

<field
    name="created_by"
    type="sql"
    sql_select="a.id, a.created_by, u.id, u.lastResetTime AS name"
    sql_from="#__content AS a"
    sql_join="#__users AS u ON a.created_by = u.id"
    sql_where="u.id != 0"
    sql_order="u.name ASC"
    key_field="created_by"
    value_field="name"
    onchange="this.form.submit();"
    >
            <option value="">COM_OCHCLICKTHIS_CLICKEDLINKS_LINK_FILTER</option>
</field>
Hackwar commented 1 month ago

The problem isn't so much the class you mentioned, but the classes using this class. They are handing in empty entries, which is triggering this error.

Ruud68 commented 1 month ago

Hi @Hackwar thanks for following up and agree. But long time ago I have learned never to trust input passed by a user. That should always be checked. So instead of not checking and in this case throw a warning (which might turn into an error in future PHP versions) It should IMO check and handle the error. The thing here is that the data comes from another Joomla core method that can correctly output null as value (nothing selected). So when the class using this class should be changed then the sql field should be changed e.g. by having a default value when the query result is null