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

Do not show a deprecated message when no ordering is set in back end list views #39050

Merged
merged 3 commits into from
Oct 28, 2022

Conversation

MacJoom
Copy link
Contributor

@MacJoom MacJoom commented Oct 23, 2022

Pull Request for Deprecated message in Joomla Patch Tester

Summary of Changes

Avoid deprecated message

Testing Instructions

Install Joomla Patch Tester

PHP 8.1
Turn on all PHP Warnings
display_errors = On
error_reporting = E_ALL | E_STRICT

Actual result BEFORE applying this Pull Request

Deprecated: strtoupper(): Passing null to parameter #1 ($string) of type string is deprecated in /var/www/clients/client2/web14/web/joomla-cms/libraries/src/MVC/Model/ListModel.php on line 543

Disturbs 'Fetch Data' in popup window too.

Expected result AFTER applying this Pull Request

No warning

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@toivo
Copy link
Contributor

toivo commented Oct 24, 2022

I have tested this item ✅ successfully on bb1ffff

Tested successfully in Joomla 4.2.4-dev of 24 October in Wampserver using PHP 8.1.10


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

@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on bb1ffff


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

@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 27, 2022
@zero-24
Copy link
Contributor

zero-24 commented Oct 27, 2022

To me this is the wrong solution for this problem.. When this is an issue only with the patchtester it should be patched in the patchtester.. When its a general problem it should be fixed for all cases not only this specific case.. Please do not merge this as it is right now.

@richard67
Copy link
Member

To me this is the wrong solution for this problem.. When this is an issue only with the patchtester it should be patched in the patchtester.. When its a general problem it should be fixed for all cases not only this specific case.. Please do not merge this as it is right now.

I think it could be a general issue because the ordering direction might not be always set. But I might be wrong of course.

@laoneo
Copy link
Member

laoneo commented Oct 27, 2022

I have the same issue in DPCalendar. For years we could pass NULL as ordering, why should it all of the sudden fail. This should be fixed here and not on every extension.

@brianteeman
Copy link
Contributor

The title of the pr should be updated to reflect what it actually is for.

@laoneo laoneo changed the title Patch Tester: Deprecated message in libraries/src/MVC/Model/ListModel.php Do not show a deprecated message when no ordering is set in back end list views Oct 27, 2022
@laoneo
Copy link
Member

laoneo commented Oct 27, 2022

Done.

@zero-24
Copy link
Contributor

zero-24 commented Oct 27, 2022

I have the same issue in DPCalendar. For years we could pass NULL as ordering, why should it all of the sudden fail. This should be fixed here and not on every extension.

It has been changed by PHP not us, so to me its a clear php combat patch of the one who is doing the no longer allowed call. When it would be patched here we should fix it for all cases in this method not just that single case that where hit by patchtester or dpcalendar.

I personally dont like us adding to all methods an is_null check as thats sound very wrong to me and is a lot of code which is avoidable but when we want this to be fixed in the Core code we should not only fix it for this sub-case but all affected places in this method.

@HLeithner
Copy link
Member

I personally dont like us adding to all methods an is_null check as thats sound very wrong to me and is a lot of code which is avoidable but when we want this to be fixed in the Core code we should not only fix it for this sub-case but all affected places in this method.

In theory you are right but this case is diffrent because the input hasn't been provided thru a mechanism which is (or can be) automatically validated by php.
The programmer provides us an ARRAY with information, and we should (have to) validate this input, in this case it's even worse because we don't validate user input (getUserStateFromRequest).

So it's perfectly fine for us to check if the provided parameters are right.

@MacJoom thanks

@HLeithner HLeithner merged commit babe508 into joomla:4.2-dev Oct 28, 2022
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 28, 2022
@richard67
Copy link
Member

Thanks @HLeithner .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants