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

[#33307] Only run processLimit when a limit or offset is set #3602

Closed
wants to merge 1 commit into from

Conversation

sovainfo
Copy link
Contributor

Fix for [#33307]: Error displaying the error page: Application Instantiation Error
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33307&start=0

@BugsFinder
Copy link

ok, PR resolves my issue.

@sovainfo
Copy link
Contributor Author

How embarrassing that this still not implemented! This is the first thing people have to do after installing J336 going to the administrator. It reports:

42000 [Microsoft][SQL Server Native Client 11.0][SQL Server]A nested INSERT, UPDATE, DELETE, or MERGE statement must have an OUTPUT clause.SQL=SELECT * FROM ( UPDATE [j336_extensions] SET [params] = '{"mediaversion":"a0029d5a3035565db1f2f0ddb93da8a6"}' WHERE [type] = 'library' AND [element] = 'joomla') _myResults WHERE RowNumber BETWEEN 1 AND 0

Which is caused by calling processLimit while it shouldn't!

Apply PR and you can login!

Maybe someone can change the prority to CRITCAL, or even better commit!

@mbabker
Copy link
Contributor

mbabker commented Oct 20, 2014

It's easy to just merge things without review or testing, but that's bad for quality assurance. Unfortunately, this has had next to no testing (there's been one good test so far). Find one or two others to test general use cases since you've modified the base query building class and we can get it merged.

@sovainfo
Copy link
Contributor Author

This has been tested and proven to work numerous times in the field. Just visit the forum!
And you know it works! You implemented the same construct in the sql driver. Don't hide behind procedures.

@mbabker
Copy link
Contributor

mbabker commented Oct 20, 2014

Any of them report a good test on any of the patches you've submitted? I can't speak for others, but I personally don't spend a lot of time in the forums and when I do it's usually not relating to bug activity anymore. Not trying to sound difficult, I promise.

@sovainfo
Copy link
Contributor Author

See http://forum.joomla.org/viewtopic.php?f=717&t=825222&hilit=+3602#p3113359
quote: Further digging & debugging showed that the function 'processLimit()' in "libraries\joomla\database\query\sqlsrv.php" was mangling the queries. Adding the parameter check that the function 'limit()' in "libraries\joomla\database[drive]\sqlsrv.php" has, relieved the "Application Instantiation Error" issue.

http://forum.joomla.org/viewtopic.php?f=717&t=825222&hilit=+3602#p3113437

Requested several times to report a good test. Seems I am not the only one that didn't like the JC Issue reporting. Even now with issues.joomla.org and Github it seems to much trouble!

Then again, for a no-brainer like this!

@Bakual
Copy link
Contributor

Bakual commented Oct 20, 2014

Then again, for a no-brainer like this!

Usually, the no-brainers break stuff.

Actually, I think you're trying to solve this at the wrong place. Since it works for PostgreSQL and MySQL, I suspect the issue in the MS SQL query class.
Looking at the MySQLi one, I see a similar check done there:
https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/database/query/mysqli.php#L46
PostgreSQL has also a bit a different check:
https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/database/query/postgresql.php#L582

MSSQL on the other hand has no such checks and looks quite a bit different:
https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/database/query/sqlsrv.php#L263

Thus I'd say fix the MSSQL one, and don't mess with the query itself where you possibly break something for the other drivers.
If you only make a change in MSSQL, it would also be easier to merge based on review or forum tests. Since it will have no effect on other installations for sure.

@Bakual
Copy link
Contributor

Bakual commented Oct 21, 2014

Please see #4870

@sovainfo
Copy link
Contributor Author

In the wild it is proven not to break. If only BUGS were as easy fixed as they are introduced to the software!

You are not solving the real issue. See #4870.

@Bakual
Copy link
Contributor

Bakual commented Oct 21, 2014

You are not solving the real issue.

It does actually exactly the same thing as your PR here, but only for MSSQL while yours would affect all database drivers.

@sovainfo
Copy link
Contributor Author

You should look at the big picture and not compare individual PR's. As mentioned there are several issues, not only the one you are talking about!

@Bakual Bakual mentioned this pull request Oct 22, 2014
@roland-d
Copy link
Contributor

I do agree that the change provided in #4870 is a better one. This makes the MSSQL driver consistent with the other drivers.

Indeed, there are more issues with MSSQL but there just aren't enough people testing them. At least not here on the issue tracker.

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

@Bakual
Copy link
Contributor

Bakual commented Dec 3, 2014

Closing as fixed with #5293

@Bakual Bakual closed this Dec 3, 2014
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

7 participants