Changeset View
Standalone View
core/units/logs/session_logs/session_log_eh.php
| Show First 20 Lines • Show All 123 Lines • ▼ Show 20 Line(s) | |||||
| if ( $related_ids ) { | if ( $related_ids ) { | ||||
| $temp_handler = $this->Application->recallObject('change-log_TempHandler', 'kTempTablesHandler', Array ('parent_event' => $event->MasterEvent)); | $temp_handler = $this->Application->recallObject('change-log_TempHandler', 'kTempTablesHandler', Array ('parent_event' => $event->MasterEvent)); | ||||
| /* @var $temp_handler kTempTablesHandler */ | /* @var $temp_handler kTempTablesHandler */ | ||||
| $temp_handler->DeleteItems('change-log', '', $related_ids); | $temp_handler->DeleteItems('change-log', '', $related_ids); | ||||
| } | } | ||||
| } | } | ||||
| } | /** | ||||
| No newline at end of file | * [SCHEDULED TASK] Will remove old session logs | ||||
| * | |||||
| * @param kEvent $event Event. | |||||
| * | |||||
| * @return void | |||||
| */ | |||||
| protected function OnRotate(kEvent $event) | |||||
| { | |||||
| $rotation_interval = (int)$this->Application->ConfigValue('SessionLogRotationInterval'); | |||||
| if ( $rotation_interval === -1 ) { | |||||
| // Forever. | |||||
| return; | |||||
| } | |||||
| $limit = 1; | |||||
alex: With such a small limit there will be a lot of database queries (one per database record)… | |||||
| $may_have_session_log = 1; | |||||
alexUnsubmitted Please use boolean data type here (e.g. true and false) not 1 and 0. The variable value doesn't represent quantity, but integer data type value is being used. alex: Please use boolean data type here (e.g. `true` and `false`) not `1` and `0`.
---
The variable… | |||||
| $config = $event->getUnitConfig(); | |||||
alexUnsubmitted Please rename to $session_log_config. Makes code easier to read. alex: Please rename to `$session_log_config`.
---
Makes code easier to read. | |||||
| $change_log_config = $this->Application->getUnitConfig('change-log'); | |||||
alexUnsubmitted Please move variable closer (but above cycle) to the usage point. Variable is declared too far ahead of it's usage place. alex: Please move variable closer (but above cycle) to the usage point.
---
Variable is declared… | |||||
| $select_sql = ' SELECT ' . $config->getIDField() . ' | |||||
Please add an empty line below this line. To logically group connected variables together. alex: Please add an empty line below this line.
---
To logically group connected variables together. | |||||
| FROM ' . $config->getTableName() . ' | |||||
| WHERE ' . TIMENOW . ' - SessionEnd > ' . $rotation_interval . ' | |||||
| LIMIT 0,' . $limit; | |||||
alexUnsubmitted Please rename to $session_log_select_sql. Makes code easier to read. alex: Please rename to `$session_log_select_sql`.
---
Makes code easier to read. | |||||
The SQL statement part isn't properly indented:  The SQL in the $change_log_select_sql variable below however is indented correctly:  alex: The SQL statement part isn't properly indented:
{F48289}
The SQL in the… | |||||
| /** @var kTempTablesHandler $temp_handler */ | |||||
| $temp_handler = $this->Application->recallObject( | |||||
| $event->getPrefixSpecial() . '_TempHandler', | |||||
| 'kTempTablesHandler', | |||||
| array('parent_event' => $event) | |||||
| ); | |||||
alexUnsubmitted Please rename to $session_log_temp_handler. Makes code easier to read. alex: Please rename to `$session_log_temp_handler`.
---
Makes code easier to read. | |||||
| /** @var kTempTablesHandler $change_log_temp_handler */ | |||||
Please use inverted condition to leave the cycle (via break;) right away. IF statement that covers 100% of the cycle scope (code inside cycle). alex: Please use inverted condition to leave the cycle (via `break;`) right away.
---
IF statement… | |||||
| $change_log_temp_handler = $this->Application->recallObject( | |||||
| 'change-log_TempHandler', | |||||
| 'kTempTablesHandler', | |||||
| array('parent_event' => $event) | |||||
| ); | |||||
alexUnsubmitted Please:
Makes code easier to read. alex: Please:
# create helper, that would accept prefix_special & event and return matching temp… | |||||
| while ( $may_have_session_log ) { | |||||
alexUnsubmitted Please:
The do ... while variation of while cycle is best to be used, when cycle condition needs to be checked after iteration. alex: Please:
# transform into `do ... while` cycle
# remove the `$may_have_session_log`
---
The… | |||||
| $ids = $this->Conn->GetCol($select_sql); | |||||
alexUnsubmitted Please rename $ids to $session_log_ids. Makes code easier to read. alex: Please rename `$ids` to `$session_log_ids`.
---
Makes code easier to read. | |||||
| if ( $ids ) { | |||||
| $may_have_change_log = 1; | |||||
alex: Do not change anything here please. | |||||
| $change_log_select_sql = ' SELECT ' . $change_log_config->getIDField() . ' | |||||
| FROM ' . $change_log_config->getTableName() . ' | |||||
Please inline this variable. Cycle condition already means I will iterate again when condition matches. There is no need to create temp variable to describe how PHP language constructs work. alex: Please inline this variable.
---
Cycle condition already means `I will iterate again when… | |||||
alex: This wasn't addressed in previous fix. | |||||
erik: Not clear why inline this variable - it is used in three places. | |||||
The comment is a reminder about previous commit, which is marked with grey color, because it references previous code version. I'm talking about $change_log_need_more_iteration variable here. alex: The comment is a reminder about previous commit, which is marked with grey color, because it… | |||||
| WHERE SessionLogId IN (' . implode(',', $ids) . ') | |||||
| LIMIT 0,' . $limit; | |||||
| while ( $may_have_change_log ) { | |||||
| $change_log_ids = $this->Conn->GetCol($change_log_select_sql); | |||||
Please inline this variable. Cycle condition already means I will iterate again when condition matches. There is no need to create temp variable to describe how PHP language constructs work. alex: Please inline this variable.
---
Cycle condition already means `I will iterate again when… | |||||
alex: This wasn't addressed in previous fix. | |||||
Inline of this variable produces code style error - can't use count() function in the cycle condition. erik: Inline of this variable produces code style error - can't use count() function in the cycle… | |||||
That's ok. You still can change it. I think that this is an error in the sniff itself, that reports that line as error. I've reported it as https://github.com/squizlabs/PHP_CodeSniffer/issues/764 . alex: That's ok. You still can change it. I think that this is an error in the sniff itself, that… | |||||
| if ( $change_log_ids ) { | |||||
| $change_log_temp_handler->DeleteItems('change-log', '', $change_log_ids); | |||||
| } | |||||
| if ( count($change_log_ids) < $limit ) { | |||||
| $may_have_change_log = 0; | |||||
| } | |||||
| } | |||||
alexUnsubmitted Please:
Code might look like this: do { $change_log_ids = $this->Conn->GetCol($change_log_select_sql); if ( $change_log_ids ) { $change_log_temp_handler->DeleteItems('change-log', '', $change_log_ids); } } while ( count($change_log_ids) == $limit ); The do ... while variation of while cycle is best to be used, when cycle condition needs to be checked after iteration. alex: Please:
# transform into `do ... while` cycle
# remove the `$may_have_change_log`
Code might… | |||||
| $temp_handler->DeleteItems($event->Prefix, $event->Special, $ids); | |||||
| } | |||||
| if ( count($ids) < $limit ) { | |||||
| $may_have_session_log = 0; | |||||
| } | |||||
| } | |||||
| } | |||||
| } | |||||
Lint: CodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBrace Expected 0 spaces before closing brace; 4 found Lint: CodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBrace: Expected 0 spaces before closing brace; 4 found | |||||
With such a small limit there will be a lot of database queries (one per database record) executed.