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

[5.1] com_installer getListQuery #42140

Merged
merged 2 commits into from Oct 24, 2023
Merged

Conversation

brianteeman
Copy link
Contributor

Summary of Changes

Remove unnecessary duplicated condition

In the method to get the database query in administrator\components\com_installer\src\Model\UpdateModel.php a query is constructed that has a duplicated condition where($db->quoteName('u.extension_id') . ' != 0'

It is first added

->where($db->quoteName('u.extension_id') . ' != 0');

and then added again

$query->where($db->quoteName('u.extension_id') . ' != 0')

Testing Instructions

Install an old extension and check for updates with debug on to see the queries

Actual result BEFORE applying this Pull Request

Query 1

SELECT u.*,`e`.`manifest_cache`
FROM `euj03_updates` AS `u`
LEFT JOIN `euj03_extensions` AS `e` ON `e`.`extension_id` = `u`.`extension_id`
WHERE `u`.`extension_id` != 0 AND `u`.`extension_id` != 0 AND `u`.`extension_id` != :eid
ORDER BY `u`.`name` ASC LIMIT 25

Query 2

SELECT COUNT(*)
FROM `euj03_updates` AS `u`
LEFT JOIN `euj03_extensions` AS `e` ON `e`.`extension_id` = `u`.`extension_id`
WHERE `u`.`extension_id` != 0 AND `u`.`extension_id` != 0 AND `u`.`extension_id` != :eid

Expected result AFTER applying this Pull Request

Query 1

SELECT u.*,`e`.`manifest_cache`
FROM `euj03_updates` AS `u`
LEFT JOIN `euj03_extensions` AS `e` ON `e`.`extension_id` = `u`.`extension_id`
WHERE `u`.`extension_id` != 0 AND `u`.`extension_id` != :eid
ORDER BY `u`.`name` ASC LIMIT 25

Query 2

SELECT COUNT(*)
FROM `euj03_updates` AS `u`
LEFT JOIN `euj03_extensions` AS `e` ON `e`.`extension_id` = `u`.`extension_id`
WHERE `u`.`extension_id` != 0 AND `u`.`extension_id` != :eid

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

@HLeithner HLeithner changed the title [5.0] com_installer getListQuery [5.1] com_installer getListQuery Oct 16, 2023
@HLeithner HLeithner changed the base branch from 5.0-dev to 5.1-dev October 16, 2023 08:12
@HLeithner
Copy link
Member

I rebased it to 5.1 since it's not a bugfix.

@brianteeman
Copy link
Contributor Author

What is it then if its not a bug?

@rdeutz
Copy link
Contributor

rdeutz commented Oct 16, 2023

your change doesn't change the query result, so it is a optimisation and not a bug fix

@brianteeman
Copy link
Contributor Author

your logic escapes me

@richard67
Copy link
Member

richard67 commented Oct 16, 2023

The first "where" which is removed by this PR is added when there is an $extensionId = $this->getState('filter.extension_id'); given, and the 2nd one later below when there is a search with prefix eid: done in the text search field.

So to be sure that it breaks nothing it would need to test all cases, list view without any filter, with filter by extension ID in the filter tools but no search in the search box, then with search in the search box without any filter by extension ID in the filter tools, and finally with a combination of both, the filter tools and the search box. And for the search box we have 2 cases, with eid: and without eid:.

The testing instructions do not mention any of these cases.

@brianteeman
Copy link
Contributor Author

The first condition is always applied. The second condition is conditionally applied. So you either get the condition once or twice. You can never have a situation where the condition is not applied. This PR is removing the second condition not the first

@richard67
Copy link
Member

The first condition is always applied. The second condition is conditionally applied. So you either get the condition once or twice. You can never have a situation where the condition is not applied. This PR is removing the second condition not the first

I see now. You are right. By review this PR seems right to me.

@Quy
Copy link
Contributor

Quy commented Oct 19, 2023

I have tested this item ✅ successfully on c5f096a

Nice catch!


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

@alikon
Copy link
Contributor

alikon commented Oct 20, 2023

I have tested this item ✅ successfully on c5f096a


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

@alikon
Copy link
Contributor

alikon commented Oct 20, 2023

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 20, 2023
@brianteeman

This comment was marked as abuse.

@HLeithner HLeithner merged commit 6d923f5 into joomla:5.1-dev Oct 24, 2023
0 of 2 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 24, 2023
@brianteeman brianteeman deleted the update_query branch October 24, 2023 13:07
@Razzo1987 Razzo1987 added this to the Joomla! 5.1.0 milestone Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants