[#12514] - Ensure query is only run if items are found to avoid database exception
- Fixed in Code Base
- 23 Oct 2016
- Medium
- Build: staging
- # 12514
- Diff
- PhilETaylor:patch-1
- Success continuous-integration/drone the build was successful Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed. Details
User tests: Successful: Unsuccessful:
Pull Request for Issue #11969 and other fringe cases
(Alternative solution to #12508 to retain performance gains recently made)
Summary of Changes
This PR is the result of debugging on two live sites with deadly fatal exceptions as per #11969
The error this PR resolved is one like:
"jos-Error: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')' at line 3 SQL=UPDATE #__banners SET impmade = (impmade + 1) WHERE id IN ()"
On code review it was clear this code was low quality, running a sql query where there were no items to update, but yet we still tried to execute a query when there were no items to increment impmade to.. this causes a deadly JDatabaseException because the sql was invalid (as the result of the implode of empty array $bid),
returning out of the method when there are no items to update stops this exception firing
Testing Instructions
Find a site where you get the deadly issue reported - apply patch - see its fixed - check that banners impressions are still logged when other banners are displayed correctly.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Labels |
Added:
?
|
||
| Category | ⇒ | Front End Components |
ok this is why I rarely contribute small items nowadays - there are a million ways to skin a cat and it doesn't matter which way a cat is skinned - as long as it is skinned.
I have now done it the way you want.
ok this is why I rarely contribute small items nowadays - there are a million ways to skin a cat and it doesn't matter which way a cat is skinned - as long as it is skinned.
i just make suggestions. i don't mandate nothing. If you want to follow then fine, if not, fine for me too.
Please don't be somewhat harassed by that
@andrepereiradasilva Not at all harassed :-) just glad this fringe case has finally been resolved, rather than a lot of people saying they cant replicate it, when clearly a handful of sites are having real issues with it :-) Plus Im high on sugar after eating waffles and icecream with the kid and her mates...
updated opening text of PR to be more clear.
despite i'm still not able to reproduce these
fringe cases
even if i suspect the root of the issue is elsewhere cause the impress() is called only when banners
https://github.com/joomla/joomla-cms/blob/080111765a6798999b739664bafa3fb5dc4ef039/modules/mod_banners/helper.php#L46
fine for me
I have tested this item
on code review
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12514.
I have tested this item
i was unable to replicate the issue, but i see no problem with this check if solves fringe cases.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12514.
Merging now for 3.7 thanks!
| Status | Pending | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-10-23 15:16:13 |
| Closed_By | ⇒ | zero-24 |
@andrepereiradasilva This is the replacement PR, only running the impmade increment sql if there are actually items to implode into the sql, because in these fringe cases, $items can actually be nil.