-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
Enable 'SQL_BIG_SELECT' #266
base: 2.0-dev
Are you sure you want to change the base?
Conversation
Enabled this to support certain issues (esprecially when upgrading Joomla) with providers that have this setting disabled by default.
Forgot to mention it was needed for mysqli databases only. |
@stephan-ansems You mean it's not needed for MySQL databases or MariaDB databases with the PDO driver https://github.com/joomla-framework/database/blob/2.0-dev/src/Mysql/MysqlDriver.php#L150 ? Or have you just not checked with "MySQL (PDO)"? |
@@ -292,6 +292,9 @@ public function connect() | |||
// And read the real sql mode to mitigate changes in mysql > 5.7.+ | |||
$this->options['sqlModes'] = explode(',', $this->setQuery('SELECT @@SESSION.sql_mode;')->loadResult()); | |||
|
|||
// Turn 'big selects' on to ensure certain selects work on platforms that try to prevent these... | |||
$this->connection->query("SET @@SESSION.sql_big_selects = 1;" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$this->connection->query("SET @@SESSION.sql_big_selects = 1;" ); | |
$this->connection->query('SET @@SESSION.sql_big_selects = 1;'); |
Code style fix.
Related CMS issues are joomla/joomla-cms#39479 and joomla/joomla-cms#41156 . |
I'm not in favor doing any request just because a handful providers are unable to configure there sql server properly. Each database request is expensive and it should be keep to a minimum. |
@stephan-ansems Meanwhile we know from a user who has tested it that it needs the change also for the PDO driver. See joomla/joomla-cms#41156 (comment) . Could you do that, too? Thanks in advance. |
@HLeithner We already set the SQL mode with every connect, so one more |
every request is a full roundtrip (actually multiple), where do we set the SQL mode? the code looks like it's only set if explicited wished by the application. if adding this then maybe as option in the application (cms) for broken server but in the end we shouldn't support this |
I didn't say that this is the case, what I say is that the framework package should be application agnostic, and the application is responsible for the concrete parameters it needs to work with the database. |
@HLeithner I've prepared 2 alternative PR's, #284 for 2.0-dev and #285 for 3.x-dev. Would that be better and should we do it in both branches or only in 3.x-dev for Joomla 5? I think we should also fix it for Joomla 4.4 and so in the 2.0-dev branch here. |
thanks, looking at the reason for the sqlMode query I found this commit, do we still need this with mysql 8? |
@HLeithner Well, the comment says |
the question is, is it still relevant for us, since in all supported version NULL dates are supported and 0000-.... is deprecated and not used in the cms anymore, not sure if this is relevant for 3rd party |
@HLeithner What does the |
There reason for the sqlModes select query is only because of this nullDate check. |
@HLeithner Possibly, but it's still not related to the issue handled here or in my draft PR's. P.S.: And as long as the framework minimum version is not adjusted we still need it as it should work with 5.6. See here: https://github.com/joomla-framework/database/blob/2.0-dev/src/Mysqli/MysqliDriver.php#L86 |
The reason I'm bringing it up here is to think about a conceptional problem. We just fix a problem of a group of people, with patchwork. But we don't have a framework which allows us or the user to set everything in easily or completely ignore the case and set it in the relevant application directly (if needed). |
@HLeithner Could you check again my 2 alternative PR's, #284 for 2.0-dev and #285 for 3.x-dev? I have modified and extended them so they handle your concerns about unnecessary SQL statements - they only do something if necessary, i.e. explicitly required by the parameters. I've also added methods to get the parameters e.g. for display in the system information in the CMS backend. The PR's are still draft because I have to provide some testing instructions (or some PR for the CMS using the stuff added here with my PR's). |
@HLeithner I've decided to split up the changes from this PR here to
I'd be happy if you could find the time and have a look on them. The same applies of course to all other readers here. |
Meanwhile I was able to identify the critical SQL query in the CMS core, and a pull request is ready for being merged: joomla/joomla-cms#42576 . So for the CMS core the issue will be solved, and it will not need the changes from this PR here or my alternative PR's which I had mentioned above. Therefore I've closed my PR's. |
Enabled this to support certain issues (especially when upgrading Joomla) with providers that have this setting disabled by default.
I experienced issues when upgrading a vanilla version of Joomla 4.1.4 to 4.1.5 which is due to the provider having disabled the 'SQL_BIG_SELECT' by default. Because this was disabled an error occurred when attempting to upgrade Joomla to the newer version. Combine this with the insistent upgrade emails from many installations at the same provider and it it gets old real fast....
Enabling this setting resolves the bug.
Summary of Changes
Only one line of code was needed
Testing Instructions
Requires some doing to set things up, but a vanilla upgrade of Joomla CMS 4.1.4 to 4.1.5 on a provider with the setting disabled and this patch not installed should reproduce the issue, pathing it would be the test.
Documentation Changes Required