Skip to content

Fix loading module by ID on PostgreSQL#28278

Merged
HLeithner merged 1 commit intojoomla:stagingfrom
SharkyKZ:patch-2
Apr 14, 2020
Merged

Fix loading module by ID on PostgreSQL#28278
HLeithner merged 1 commit intojoomla:stagingfrom
SharkyKZ:patch-2

Conversation

@SharkyKZ
Copy link
Contributor

@SharkyKZ SharkyKZ commented Mar 9, 2020

Summary of Changes

Fixes module loading on PostgreSQL.

Testing Instructions

This needs to be tested on PostgreSQL.

Insert a module into an article.
View the article in frontend.

Expected result

Module appears.

Actual result

Module doesn't appear.

Documentation Changes Required

No.

@richard67
Copy link
Member

@SharkyKZ How is that related to PR #28269 ? I see that one and this one here do concurrent changes in the same file, and I think for the same purpose, one in staging and one in 4.0-dev. Or do I understand something wrong?

@mbabker
Copy link
Contributor

mbabker commented Mar 9, 2020

The two PRs are unrelated.

@SharkyKZ
Copy link
Contributor Author

SharkyKZ commented Mar 9, 2020

It's ultimately the same issue. The issue exists in staging and should be fixed there. Also #28269 doesn't work correctly on PDO as noted in the comments. Meanwhile, this works correctly on all database drivers and will work when it reaches 4.0.

@SharkyKZ
Copy link
Contributor Author

SharkyKZ commented Mar 9, 2020

Though for 4.0 we could change argument type to integer as discussed in #26531.

@mbabker
Copy link
Contributor

mbabker commented Mar 9, 2020

FWIW this is needed regardless of whatever external facing API changes someone wants to propose. The issue here is the database results come back in an inconsistent type so typecasting is needed to not break the function’s internal flow.

How is this different than the other PR? That is aiming to change the public API of the method, this one typecasts the database results to align with the currently documented public API (so you still have to pass a string identifier in here, a non-string will fail the strict comparison).

@richard67
Copy link
Member

@SharkyKZ I've applied your patch and done as instructed but still can't see that bloody module. Am using PotsgreSQL 11 on PHP 7.3 with the native postgresql driver. Should I use the PDO driver instead? I'll try in a while.

@richard67
Copy link
Member

@SharkyKZ False alarm, used the wrong module, popular tags. Seems there are none. With Breadcrumbs it works ;-)

@richard67
Copy link
Member

I have tested this item ✅ successfully on f02c086

Hint for other testers: You have to use PostgreSQL (PDO) to reproduce the issue. With the native PostgreSQL driver the issue doesn't happen here with PostgreSQL 11.


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

@jwaisner
Copy link
Member

I have tested this item ✅ successfully on f02c086

Tested successful on PostgreSQL (PDO) 11.3


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

@jwaisner
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 14, 2020
@richard67
Copy link
Member

I've just tested that nothing is broken for MySQLi and MySQL (PDO). All ok.

@HLeithner HLeithner merged commit e2c1eca into joomla:staging Apr 14, 2020
@HLeithner
Copy link
Member

Thanks

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Apr 14, 2020
@HLeithner HLeithner added this to the Joomla! 3.9.17 milestone Apr 14, 2020
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.

6 participants