The SQL query that was being generated was messed up. I've attempted … #3191

Merged
merged 6 commits into from Mar 9, 2016

Projects

None yet

4 participants

@geordish
Contributor
geordish commented Mar 8, 2016

…to refactor the way the query gets generated. Not 100% certain this is the best way to go about it

Fix for #3183

@geordish geordish The SQL query that was being generated was messed up. I've attempted …
…to refactor the way the query gets generated. Not 100% certain this is the best way to go about it
f381ada
@geordish geordish I agree to the conditions of the Contributor Agreement
 contained in doc/General/Contributing.md.
0d8ba52
geordish added some commits Mar 8, 2016
@geordish geordish Can't push to an empty array
a09d314
@geordish geordish Fix for admin users
6b4d585
@ekoyle
Contributor
ekoyle commented Mar 8, 2016

@geordish: can you provide the incorrect SQL that was generated?

@ekoyle
Contributor
ekoyle commented Mar 8, 2016

I think the only change that would have any affect here is the added space between P on 33 and WHERE on line 34... changing the where clauses looks unnecessary.

@geordish
Contributor
geordish commented Mar 8, 2016

Line 35 and 36 would produce something like

WHERE S.device_id = P.device_id AND P.user_id = ?1

With that extra 1 being introduced by the $where var.

On second thoughts, it may have been simpler to do the following instead.

$sql .= 'WHERE ' . $where . ' AND S.device_id = P.device_id AND P.user_id =
? ';

Would result in a simpler change.

If that is the preferred method I will fix up the PR tomorrow.
On 8 Mar 2016 20:48, "Eldon Koyle" notifications@github.com wrote:

I think the only change that would have any affect here is the added space
between P on 33 and WHERE on line 34... changing the where clauses looks
unnecessary.


Reply to this email directly or view it on GitHub
#3191 (comment).

@laf laf commented on an outdated diff Mar 8, 2016
html/includes/table/syslog.inc.php
@@ -75,5 +77,6 @@
'rowCount' => $rowCount,
'rows' => $response,
'total' => $total,
+'sql' => $sql,
@laf
laf Mar 8, 2016 Member

Looks like you've left this in from debugging. Can you remove please :)

Also, you can use logfile($sql); and it will write to logs/librenms.log if you need to.

@ekoyle
Contributor
ekoyle commented Mar 8, 2016

Something like the following would be much simpler. Would this still work?

diff --git a/html/includes/table/syslog.inc.php b/html/includes/table/syslog.inc.php
index 987aac6..c7272fe 100644
--- a/html/includes/table/syslog.inc.php
+++ b/html/includes/table/syslog.inc.php
@@ -31,8 +31,8 @@ if ($_SESSION['userlevel'] >= '5') {
     $sql .= ' WHERE '.$where;
 }
 else {
-    $sql   = 'FROM syslog AS S, devices_perms AS P';
-    $sql  .= 'WHERE S.device_id = P.device_id AND P.user_id = ?';
+    $sql   = 'FROM syslog AS S, devices_perms AS P ';
+    $sql  .= 'WHERE S.device_id = P.device_id AND P.user_id = ? AND ';
     $sql  .= $where;
     $param = array_merge(array($_SESSION['user_id']), $param);
 }
@ekoyle
Contributor
ekoyle commented Mar 9, 2016

@geordish: want to update your PR, or would you like me to submit a separate PR with the simplified patch (still needs testing)?

@geordish
Contributor
geordish commented Mar 9, 2016

@ekoyle don't worry, I will update it later. Been super busy today!

geordish added some commits Mar 9, 2016
@geordish geordish Changing to a simpler fix suggested by @ekoyle 39aaf84
@geordish geordish Merge branch 'master' of https://github.com/geordish/librenms into is…
…sue-3183
d804292
@laf laf removed the Blocker label Mar 9, 2016
@laf laf merged commit 33710fa into librenms:master Mar 9, 2016

3 checks passed

Auto-Deploy Build finished. No test results found.
Details
Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@geordish geordish deleted the geordish:issue-3183 branch Oct 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment