[4.0] Add prepared statements for plg_content_contact by HLeithner · Pull Request #25052 · joomla/joomla-cms

HLeithner · 2019-05-29T20:04:47Z

Summary of Changes

Updated SQL queries to prepared statements and made some cleanups around the queries.

Testing Instructions

Use the plugin in all ways you can think of.

Create users
Create contacts linked to the users
Create articles linked to different users
Contact should be shown at the article
use different settings in the plugin

Expected result

Nothing changed.

…tent-joomla

…plg-content-joomla

…omla-cms into prepared-plg-content-contact

plugins/content/joomla/joomla.php

plugins/content/contact/contact.php

plugins/content/joomla/joomla.php

plugins/content/contact/contact.php

Co-Authored-By: Quy <quy@fluxbb.org>

wilsonge · 2019-10-09T22:22:20Z

Conflicts please

…plg-content-contact # Conflicts: # plugins/content/contact/contact.php

…ntact-fix-sql PHPCS

richard67 · 2019-10-20T13:02:48Z

All tests passed in Drone, including all mysql8 and postgresql related 😄

richard67 · 2019-10-20T13:08:47Z

I have tested this item ✅ successfully on c2760ad

Tested with MySQL 5.7 and PostgreSQL 10.10.

@waader Could you test this PR again and see if you still have this issue you've mentioned in your comment #25052 (comment)? It should be fixed now. The server variable setiing is not exotic, it is more standard. Before MySQL 8, the default value was for compatibility with older MySQL versions, but with MySQL 8 they want to have default settings closed to standards, that's why issues appear more with 8. I would be happy if you could find some time for a test. Thanks in advance.

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

waader · 2019-10-20T15:32:24Z

The error message changed a little bit to:
1140 In aggregated query without GROUP BY, expression #2 of SELECT list contains nonaggregated column 'joomla4.contact_2.alias'; this is incompatible with sql_mode=only_full_group_by

Tested with mysql 8.0.17

richard67 · 2019-10-20T15:38:39Z

@waader @HLeithner @wilsonge Hmm, it seems then I have to change it to use a subquery. Stay tuned, it may take a while because have other stuff to do right now.

richard67 · 2019-10-20T16:34:11Z

@SharkyKZ Do you have an idea how to change the SQL statement handled with this PR to using a subquery in an elegant way?

SharkyKZ · 2019-10-20T17:12:15Z

We don't support ONLY_FULL_GROUP_BY. See #12494 for reference.

richard67 · 2019-10-20T17:31:51Z

But I think we could fix it here at least by using a subquery for the "contact_1" part in that statement.

SharkyKZ · 2019-10-20T17:34:17Z

I think MAX() can be removed. Instead order by ID descending and set 1 limit.

richard67 · 2019-10-20T17:37:31Z

I think MAX() can be removed. Instead order by ID descending and set 1 limit.

@SharkyKZ Good idea. Could you make a code proposal here as comment or a PR against Harald's branch? That would be fastest solution. I would have to look that up, limit and so on.

SharkyKZ · 2019-10-20T17:47:06Z

OK, I will. This will also correct the issue where data from different contacts is being selected. I think the idea here was to use the newest contact.

SharkyKZ · 2019-10-20T17:59:18Z

Submitted HLeithner#30.

[plg_content_contact] Remove unnecessary MAX() use

SharkyKZ · 2019-10-21T05:17:10Z

plugins/content/contact/contact.php

-					$db->quoteName('contact.webpage'),
-					$db->quoteName('contact.email_to'),
-				]
+		$query->select($db->quoteName('contact.id', 'contactid'))


This is actually doing the opposite of reducing function calls. Joomla\Database\DatabaseDriver::quoteName() is written in a recursive manner. If array is passed, it runs additional checks and then calls quoteName() again for each array element.

This is actually doing the opposite of reducing function calls. Joomla\Database\DatabaseDriver::quoteName() is written in a recursive manner. If array is passed, it runs additional checks and then calls quoteName() again for each array element.

You are right but I think the quoteName function should be optimized.

richard67 · 2019-10-21T18:28:25Z

@waader Sorry to bother you again. Could you test this PR again? With the last change it should work now. I will test, too, but because I don't have MySQL 8 I could not reproduce the issue you had.

richard67 · 2019-10-21T18:35:40Z

I have tested this item ✅ successfully on acbbd4e

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

waader · 2019-10-21T19:32:47Z

I have tested this item ✅ successfully on acbbd4e

Works now for mysql8 and postgresql. Thank you Richard et al.

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

richard67 · 2019-10-21T19:34:40Z

Thanks to @HLeithner and @SharkyKZ , they fixed it. I've only tried and failed.

SharkyKZ · 2019-10-21T19:50:04Z

RTC

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

wilsonge · 2019-10-21T20:46:18Z

Good teamwork guys and thanks for the test spotting this @waader !

HLeithner · 2019-10-21T20:47:29Z

thx for testing and fixing!

HLeithner added 9 commits May 5, 2019 13:25

Add prepared statements for plg_content_contact

1e9ee6b

Fix namespace

f1de28e

Add prepared statements for plg_content_joomla

d0eb46a

Fix exception var typo

d11057b

cs

c16263e

cs

2e1a8f3

Merge remote-tracking branch 'upstream/4.0-dev' into prepared-plg-con…

3bc69f9

…tent-joomla

Merge branch '4.0-dev' of github.com:joomla/joomla-cms into prepared-…

fa14ab2

…plg-content-joomla

Merge branch 'prepared-plg-content-joomla' of github.com:HLeithner/jo…

c24f840

…omla-cms into prepared-plg-content-contact

joomla-cms-bot added the PR-4.0-dev label May 29, 2019

Quy reviewed May 29, 2019

View reviewed changes

plugins/content/joomla/joomla.php Outdated Show resolved Hide resolved

ghost changed the title ~~Add prepared statements for plg_content_contact~~ [4.0] Add prepared statements for plg_content_contact May 30, 2019

Add parameter type

d452537

Quy reviewed May 31, 2019

View reviewed changes

plugins/content/joomla/joomla.php Outdated Show resolved Hide resolved

Quy reviewed May 31, 2019

View reviewed changes

plugins/content/joomla/joomla.php Outdated Show resolved Hide resolved

cs

a9cb54f

Quy reviewed May 31, 2019

View reviewed changes

plugins/content/contact/contact.php Outdated Show resolved Hide resolved

Quy reviewed May 31, 2019

View reviewed changes

plugins/content/joomla/joomla.php Outdated Show resolved Hide resolved

HLeithner added 2 commits May 31, 2019 15:54

Removed unnecessary variable

ae0e5d3

Revert changes to wrong file

8ca84b3

alikon reviewed Jun 6, 2019

View reviewed changes

plugins/content/contact/contact.php Outdated Show resolved Hide resolved

alikon suggested changes Jun 6, 2019

View reviewed changes

plugins/content/contact/contact.php Outdated Show resolved Hide resolved

HLeithner added 2 commits June 9, 2019 19:38

Move typecast out of function call

88640c2

Merge branch '4.0-dev' into prepared-plg-content-contact

a0cfab0

SharkyKZ reviewed Jul 3, 2019

View reviewed changes

plugins/content/contact/contact.php Outdated Show resolved Hide resolved

Fix nested array

94caca0

Quy reviewed Jul 11, 2019

View reviewed changes

plugins/content/contact/contact.php Outdated Show resolved Hide resolved

Update plugins/content/contact/contact.php

4db74fd

Co-Authored-By: Quy <quy@fluxbb.org>

Merge branch '4.0-dev' of github.com:joomla/joomla-cms into prepared-…

bdc1f69

…plg-content-contact # Conflicts: # plugins/content/contact/contact.php

richard67 mentioned this pull request Oct 20, 2019

PHPCS HLeithner/joomla-cms#29

Merged

Merge pull request #29 from richard67/4.0-dev-prepared-plg-content-co…

c2760ad

…ntact-fix-sql PHPCS

Remove unnecessary MAX() use

4c85d9c

HLeithner added 2 commits October 20, 2019 22:26

Merge pull request #30 from SharkyKZ/patch-2

1921314

[plg_content_contact] Remove unnecessary MAX() use

Reduce function calls

1a55eb2

SharkyKZ reviewed Oct 21, 2019

View reviewed changes

Merge branch '4.0-dev' into prepared-plg-content-contact

acbbd4e

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 21, 2019

wilsonge merged commit c5d8d10 into joomla:4.0-dev Oct 21, 2019

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 21, 2019

wilsonge added this to the Joomla 4.0 milestone Oct 21, 2019

HLeithner deleted the prepared-plg-content-contact branch October 21, 2019 20:47

Uh oh!

Conversation

HLeithner commented May 29, 2019

Summary of Changes

Testing Instructions

Expected result

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wilsonge commented Oct 9, 2019

Uh oh!

richard67 commented Oct 20, 2019

Uh oh!

richard67 commented Oct 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

waader commented Oct 20, 2019

Uh oh!

richard67 commented Oct 20, 2019

Uh oh!

richard67 commented Oct 20, 2019

Uh oh!

SharkyKZ commented Oct 20, 2019

Uh oh!

richard67 commented Oct 20, 2019

Uh oh!

SharkyKZ commented Oct 20, 2019

Uh oh!

richard67 commented Oct 20, 2019

Uh oh!

SharkyKZ commented Oct 20, 2019

Uh oh!

SharkyKZ commented Oct 20, 2019

Uh oh!

SharkyKZ Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

HLeithner Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

richard67 commented Oct 21, 2019

Uh oh!

richard67 commented Oct 21, 2019

Uh oh!

waader commented Oct 21, 2019

Uh oh!

richard67 commented Oct 21, 2019

Uh oh!

SharkyKZ commented Oct 21, 2019

Uh oh!

wilsonge commented Oct 21, 2019

Uh oh!

HLeithner commented Oct 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

richard67 commented Oct 20, 2019 •

edited

Loading