From 7f695197aae9d87f5c04c2f7801d97852d72a3bc Mon Sep 17 00:00:00 2001 From: fabrixxm Date: Fri, 20 Aug 2021 09:47:53 +0200 Subject: [PATCH] Fix review points - Fix headers hierarchy - Improve accessibility: - set mouse pointer - make rows focusable - open on key press - add tooltip with "title" - add role and aria attributes - Rename `ParsedLog` to `ParsedLogLine` - Add docs to `ReversedFileReader`'s implementation of `Iterator`'s methods - Add docs to `ParsedLogIterator`'s implementation of `Iterator`'s methods - Remove unnecessary comment - Add more test for parsing log lines and fix some edge cases - Fix function name in snake-case to camelCase - Remove `DIRECTORY_SEPARATOR` --- src/Model/Log/ParsedLogIterator.php | 63 ++++++++---- .../Log/{ParsedLog.php => ParsedLogLine.php} | 51 ++++++---- src/Util/ReversedFileReader.php | 41 ++++++++ tests/src/Model/Log/ParsedLogIteratorTest.php | 7 +- ...arsedLogTest.php => ParsedLogLineTest.php} | 96 ++++++++++++++++++- view/js/module/admin/logs/view.js | 34 ++++++- view/templates/admin/logs/view.tpl | 12 ++- view/theme/frio/css/style.css | 7 ++ view/theme/frio/js/module/admin/logs/view.js | 17 +++- view/theme/frio/templates/admin/logs/view.tpl | 6 +- 10 files changed, 280 insertions(+), 54 deletions(-) rename src/Object/Log/{ParsedLog.php => ParsedLogLine.php} (76%) rename tests/src/Object/Log/{ParsedLogTest.php => ParsedLogLineTest.php} (54%) diff --git a/src/Model/Log/ParsedLogIterator.php b/src/Model/Log/ParsedLogIterator.php index bde07ee4a8..856edb38fd 100644 --- a/src/Model/Log/ParsedLogIterator.php +++ b/src/Model/Log/ParsedLogIterator.php @@ -22,10 +22,10 @@ namespace Friendica\Model\Log; use Friendica\Util\ReversedFileReader; -use Friendica\Object\Log\ParsedLog; +use Friendica\Object\Log\ParsedLogLine; /** - * An iterator which returns `\Friendica\Objec\Log\ParsedLog` instances + * An iterator which returns `\Friendica\Objec\Log\ParsedLogLine` instances * * Uses `\Friendica\Util\ReversedFileReader` to fetch log lines * from newest to oldest. @@ -35,7 +35,7 @@ class ParsedLogIterator implements \Iterator /** @var \Iterator */ private $reader; - /** @var ParsedLog current iterator value*/ + /** @var ParsedLogLine current iterator value*/ private $value = null; /** @var int max number of lines to read */ @@ -100,19 +100,19 @@ class ParsedLogIterator implements \Iterator * Check if parsed log line match filters. * Always match if no filters are set. * - * @param ParsedLog $parsedlog + * @param ParsedLogLine $parsedlogline * @return bool */ - private function filter($parsedlog) + private function filter($parsedlogline) { $match = true; foreach ($this->filters as $filter => $filtervalue) { switch ($filter) { case "level": - $match = $match && ($parsedlog->level == strtoupper($filtervalue)); + $match = $match && ($parsedlogline->level == strtoupper($filtervalue)); break; case "context": - $match = $match && ($parsedlog->context == $filtervalue); + $match = $match && ($parsedlogline->context == $filtervalue); break; } } @@ -123,13 +123,13 @@ class ParsedLogIterator implements \Iterator * Check if parsed log line match search. * Always match if no search query is set. * - * @param ParsedLog $parsedlog + * @param ParsedLogLine $parsedlogline * @return bool */ - private function search($parsedlog) + private function search($parsedlogline) { if ($this->search != "") { - return strstr($parsedlog->logline, $this->search) !== false; + return strstr($parsedlogline->logline, $this->search) !== false; } return true; } @@ -138,8 +138,8 @@ class ParsedLogIterator implements \Iterator * Read a line from reader and parse. * Returns null if limit is reached or the reader is invalid. * - * @param ParsedLog $parsedlog - * @return ?ParsedLog + * @param ParsedLogLine $parsedlogline + * @return ?ParsedLogLine */ private function read() { @@ -149,22 +149,34 @@ class ParsedLogIterator implements \Iterator } $line = $this->reader->current(); - return new ParsedLog($this->reader->key(), $line); + return new ParsedLogLine($this->reader->key(), $line); } + + /** + * Fetch next parsed log line which match with filters or search and + * set it as current iterator value. + * + * @see Iterator::next() + * @return void + */ public function next() { $parsed = $this->read(); - // if read() has not retuned none and - // the line don't match filters or search - // read the next line while (is_null($parsed) == false && !($this->filter($parsed) && $this->search($parsed))) { $parsed = $this->read(); } $this->value = $parsed; } + + /** + * Rewind the iterator to the first matching log line + * + * @see Iterator::rewind() + * @return void + */ public function rewind() { $this->value = null; @@ -172,16 +184,35 @@ class ParsedLogIterator implements \Iterator $this->next(); } + /** + * Return current parsed log line number + * + * @see Iterator::key() + * @see ReversedFileReader::key() + * @return int + */ public function key() { return $this->reader->key(); } + /** + * Return current iterator value + * + * @see Iterator::current() + * @return ?ParsedLogLing + */ public function current() { return $this->value; } + /** + * Checks if current iterator value is valid, that is, not null + * + * @see Iterator::valid() + * @return bool + */ public function valid() { return ! is_null($this->value); diff --git a/src/Object/Log/ParsedLog.php b/src/Object/Log/ParsedLogLine.php similarity index 76% rename from src/Object/Log/ParsedLog.php rename to src/Object/Log/ParsedLogLine.php index b1bc821125..93a15374bb 100644 --- a/src/Object/Log/ParsedLog.php +++ b/src/Object/Log/ParsedLogLine.php @@ -24,7 +24,7 @@ namespace Friendica\Object\Log; /** * Parse a log line and offer some utility methods */ -class ParsedLog +class ParsedLogLine { const REGEXP = '/^(\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}[^ ]*) (\w+) \[(\w*)\]: (.*)/'; @@ -64,16 +64,23 @@ class ParsedLog private function parse($logline) { + $this->logline = $logline; + // if data is empty is serialized as '[]'. To ease the parsing // let's replace it with '{""}'. It will be replaced by null later $logline = str_replace(' [] - {', ' {""} - {', $logline); - // here we hope that there will not be the string ' - {' inside the $jsonsource value - list($logline, $jsonsource) = explode(' - {', $logline); - $jsonsource = '{' . $jsonsource; + + if (strstr($logline, ' - {') === false) { + // the log line is not well formed + $jsonsource = null; + } else { + // here we hope that there will not be the string ' - {' inside the $jsonsource value + list($logline, $jsonsource) = explode(' - {', $logline); + $jsonsource = '{' . $jsonsource; + } $jsondata = null; - if (strpos($logline, '{"') > 0) { list($logline, $jsondata) = explode('{"', $logline, 2); @@ -82,15 +89,20 @@ class ParsedLog preg_match(self::REGEXP, $logline, $matches); - $this->date = $matches[1]; - $this->context = $matches[2]; - $this->level = $matches[3]; - $this->message = trim($matches[4]); - $this->data = $jsondata == '{""}' ? null : $jsondata; - $this->source = $jsonsource; - $this->try_fix_json(); + if (count($matches) == 0) { + // regexp not matching + $this->message = $this->logline; + } else { + $this->date = $matches[1]; + $this->context = $matches[2]; + $this->level = $matches[3]; + $this->message = $matches[4]; + $this->data = $jsondata == '{""}' ? null : $jsondata; + $this->source = $jsonsource; + $this->tryfixjson(); + } - $this->logline = $logline; + $this->message = trim($this->message); } /** @@ -101,7 +113,7 @@ class ParsedLog * This method try to parse the found json and if it fails, search for next '{' * in json data and retry */ - private function try_fix_json() + private function tryfixjson() { if (is_null($this->data) || $this->data == '') { return; @@ -112,10 +124,15 @@ class ParsedLog // try to find next { in $str and move string before to 'message' $pos = strpos($this->data, '{', 1); + if ($pos === false) { + $this->message .= $this->data; + $this->data = null; + return; + } $this->message .= substr($this->data, 0, $pos); $this->data = substr($this->data, $pos); - $this->try_fix_json(); + $this->tryfixjson(); } } @@ -124,7 +141,7 @@ class ParsedLog * * @return array */ - public function get_data() + public function getData() { $data = json_decode($this->data, true); if ($data) { @@ -140,7 +157,7 @@ class ParsedLog * * @return array */ - public function get_source() + public function getSource() { return json_decode($this->source, true); } diff --git a/src/Util/ReversedFileReader.php b/src/Util/ReversedFileReader.php index c4fe131aee..248792f17a 100644 --- a/src/Util/ReversedFileReader.php +++ b/src/Util/ReversedFileReader.php @@ -70,6 +70,11 @@ class ReversedFileReader implements \Iterator return $this; } + /** + * Read $size bytes behind last position + * + * @return string + */ private function _read($size) { $this->pos -= $size; @@ -77,6 +82,12 @@ class ReversedFileReader implements \Iterator return fread($this->fh, $size); } + /** + * Read next line from end of file + * Return null if no lines are left to read + * + * @return ?string + */ private function _readline() { $buffer = & $this->buffer; @@ -91,12 +102,24 @@ class ReversedFileReader implements \Iterator } } + /** + * Fetch next line from end and set it as current iterator value. + * + * @see Iterator::next() + * @return void + */ public function next() { ++$this->key; $this->value = $this->_readline(); } + /** + * Rewind iterator to the first line at the end of file + * + * @see Iterator::rewind() + * @return void + */ public function rewind() { if ($this->filesize > 0) { @@ -108,16 +131,34 @@ class ReversedFileReader implements \Iterator } } + /** + * Return current line number, starting from zero at the end of file + * + * @see Iterator::key() + * @return int + */ public function key() { return $this->key; } + /** + * Return current line + * + * @see Iterator::current() + * @return string + */ public function current() { return $this->value; } + /** + * Checks if current iterator value is valid, that is, we readed all lines in files + * + * @see Iterator::valid() + * @return bool + */ public function valid() { return ! is_null($this->value); diff --git a/tests/src/Model/Log/ParsedLogIteratorTest.php b/tests/src/Model/Log/ParsedLogIteratorTest.php index 846deab1ce..df9f8393a9 100644 --- a/tests/src/Model/Log/ParsedLogIteratorTest.php +++ b/tests/src/Model/Log/ParsedLogIteratorTest.php @@ -42,12 +42,7 @@ class ParsedLogIteratorTest extends TestCase protected function setUp(): void { - $logfile = dirname(__DIR__) . DIRECTORY_SEPARATOR . - '..' . DIRECTORY_SEPARATOR . - '..' . DIRECTORY_SEPARATOR . - 'datasets' . DIRECTORY_SEPARATOR . - 'log' . DIRECTORY_SEPARATOR . - 'friendica.log.txt'; + $logfile = dirname(__DIR__) . '/../../datasets/log/friendica.log.txt'; $reader = new ReversedFileReader(); $this->pli = new ParsedLogIterator($reader); diff --git a/tests/src/Object/Log/ParsedLogTest.php b/tests/src/Object/Log/ParsedLogLineTest.php similarity index 54% rename from tests/src/Object/Log/ParsedLogTest.php rename to tests/src/Object/Log/ParsedLogLineTest.php index ff3d06f55b..9705c34ea2 100644 --- a/tests/src/Object/Log/ParsedLogTest.php +++ b/tests/src/Object/Log/ParsedLogLineTest.php @@ -21,17 +21,17 @@ namespace Friendica\Test\src\Object\Log; -use Friendica\Object\Log\ParsedLog; +use Friendica\Object\Log\ParsedLogLine; use PHPUnit\Framework\TestCase; /** * Log parser testing class */ -class ParsedLogTest extends TestCase +class ParsedLogLineTest extends TestCase { public static function do_log_line($logline, $expected_data) { - $parsed = new ParsedLog(0, $logline); + $parsed = new ParsedLogLine(0, $logline); foreach ($expected_data as $k => $v) { self::assertSame($parsed->$k, $v, '"'.$k.'" does not match expectation'); } @@ -90,4 +90,94 @@ class ParsedLogTest extends TestCase ] ); } + + /** + * test non conforming log line + */ + public function testNonConformingLogLine() + { + self::do_log_line( + 'this log line is not formatted as expected', + [ + 'date' => null, + 'context' => null, + 'level' => null, + 'message' => 'this log line is not formatted as expected', + 'data' => null, + 'source' => null, + ] + ); + } + + /** + * test missing source + */ + public function testMissingSource() + { + self::do_log_line( + '2021-05-24T15:30:01Z worker [NOTICE]: Load: 0.01/20 - processes: 0/1/6 (0:0, 30:1) - maximum: 10/10 {"worker_id":"ece8fc8","worker_cmd":"Cron"}', + [ + 'date' => '2021-05-24T15:30:01Z', + 'context' => 'worker', + 'level' => 'NOTICE', + 'message' => 'Load: 0.01/20 - processes: 0/1/6 (0:0, 30:1) - maximum: 10/10', + 'data' => '{"worker_id":"ece8fc8","worker_cmd":"Cron"}', + 'source' => null, + ] + ); + } + + /** + * test missing data + */ + public function testMissingData() + { + self::do_log_line( + '2021-05-24T15:30:01Z worker [NOTICE]: Load: 0.01/20 - processes: 0/1/6 (0:0, 30:1) - maximum: 10/10 - {"file":"Worker.php","line":786,"function":"tooMuchWorkers","uid":"364d3c","process_id":20754}', + [ + 'date' => '2021-05-24T15:30:01Z', + 'context' => 'worker', + 'level' => 'NOTICE', + 'message' => 'Load: 0.01/20 - processes: 0/1/6 (0:0, 30:1) - maximum: 10/10', + 'data' => null, + 'source' => '{"file":"Worker.php","line":786,"function":"tooMuchWorkers","uid":"364d3c","process_id":20754}', + ] + ); + } + + /** + * test missing data and source + */ + public function testMissingDataAndSource() + { + self::do_log_line( + '2021-05-24T15:30:01Z worker [NOTICE]: Load: 0.01/20 - processes: 0/1/6 (0:0, 30:1) - maximum: 10/10', + [ + 'date' => '2021-05-24T15:30:01Z', + 'context' => 'worker', + 'level' => 'NOTICE', + 'message' => 'Load: 0.01/20 - processes: 0/1/6 (0:0, 30:1) - maximum: 10/10', + 'data' => null, + 'source' => null, + ] + ); + } + + /** + * test missing source and invalid data + */ + public function testMissingSourceAndInvalidData() + { + self::do_log_line( + '2021-05-24T15:30:01Z worker [NOTICE]: Load: 0.01/20 - processes: 0/1/6 (0:0, 30:1) - maximum: 10/10 {"invalidjson {really', + [ + 'date' => '2021-05-24T15:30:01Z', + 'context' => 'worker', + 'level' => 'NOTICE', + 'message' => 'Load: 0.01/20 - processes: 0/1/6 (0:0, 30:1) - maximum: 10/10 {"invalidjson {really', + 'data' => null, + 'source' => null, + ] + ); + } } diff --git a/view/js/module/admin/logs/view.js b/view/js/module/admin/logs/view.js index 45d08a5ed1..5ad4e0a717 100644 --- a/view/js/module/admin/logs/view.js +++ b/view/js/module/admin/logs/view.js @@ -1,7 +1,33 @@ -function log_show_details(id) { +(function(){ + function log_show_details(elm) { + const id = elm.id; + var hidden = true; + document + .querySelectorAll('[data-id="' + id + '"]') + .forEach(edetails => { + hidden = edetails.classList.toggle('hidden'); + }); + document + .querySelectorAll('[aria-expanded="true"]') + .forEach(eexpanded => { + eexpanded.setAttribute('aria-expanded', false); + }); + + if (!hidden) { + elm.setAttribute('aria-expanded', true); + } + } + document - .querySelectorAll('[data-id="' + id + '"]') + .querySelectorAll('.log-event') .forEach(elm => { - elm.classList.toggle('hidden') + elm.addEventListener("click", evt => { + log_show_details(evt.currentTarget); + }); + elm.addEventListener("keydown", evt => { + if (evt.keyCode == 13 || evt.keyCode == 32) { + log_show_details(evt.currentTarget); + } + }); }); -} +})(); \ No newline at end of file diff --git a/view/templates/admin/logs/view.tpl b/view/templates/admin/logs/view.tpl index 523cb6a201..e15a4a01b3 100644 --- a/view/templates/admin/logs/view.tpl +++ b/view/templates/admin/logs/view.tpl @@ -1,7 +1,7 @@

{{$title}} - {{$page}}

-

{{$logname}}

+

{{$logname}}

{{if $error }}

{{$error nofilter}}

@@ -44,14 +44,18 @@ {{foreach $data as $row}} - + {{$row->date}} {{$row->level}} {{$row->context}} {{$row->message}} Data - {{foreach $row->get_data() as $k=>$v}} + {{foreach $row->getData() as $k=>$v}} {{$k}} @@ -60,7 +64,7 @@ {{/foreach}} Source - {{foreach $row->get_source() as $k=>$v}} + {{foreach $row->getSource() as $k=>$v}} {{$k}} {{$v}} diff --git a/view/theme/frio/css/style.css b/view/theme/frio/css/style.css index 1bf908e1da..bcf1557ae6 100644 --- a/view/theme/frio/css/style.css +++ b/view/theme/frio/css/style.css @@ -98,6 +98,13 @@ details summary { display: list-item; } +/** + * clickable table rows + */ +.table > tbody > td[role="button"] { + cursor: pointer; +} + /** * mobile aside */ diff --git a/view/theme/frio/js/module/admin/logs/view.js b/view/theme/frio/js/module/admin/logs/view.js index 149d019e90..245518822f 100644 --- a/view/theme/frio/js/module/admin/logs/view.js +++ b/view/theme/frio/js/module/admin/logs/view.js @@ -21,6 +21,12 @@ $(function(){ $(".log-event").on("click", function(ev) { show_details_for_element(ev.currentTarget); }); + $(".log-event").on("keydown", function(ev) { + if (ev.keyCode == 13 || ev.keyCode == 32) { + show_details_for_element(ev.currentTarget); + } + }); + $("[data-previous").on("click", function(ev){ var currentid = document.getElementById("logdetail").dataset.rowId; @@ -37,9 +43,15 @@ $(function(){ }); - function show_details_for_element(element) { - var $modal = $("#logdetail"); + const $modal = $("#logdetail"); + $modal.on("hidden.bs.modal", function(ev){ + document + .querySelectorAll('[aria-expanded="true"]') + .forEach(elm => elm.setAttribute("aria-expanded", false)) + }); + + function show_details_for_element(element) { $modal[0].dataset.rowId = element.id; var tr = $modal.find(".main-data tbody tr")[0]; @@ -64,6 +76,7 @@ $(function(){ $("[data-next").prop("disabled", $(element).next().length == 0); $modal.modal({}) + element.setAttribute("aria-expanded", true); } function recursive_details(s, data, lev=0) { diff --git a/view/theme/frio/templates/admin/logs/view.tpl b/view/theme/frio/templates/admin/logs/view.tpl index 1bc0269160..1e1123fbd9 100755 --- a/view/theme/frio/templates/admin/logs/view.tpl +++ b/view/theme/frio/templates/admin/logs/view.tpl @@ -1,7 +1,7 @@

{{$title}} - {{$page}}

-

{{$logname}}

+

{{$logname}}

{{if $error }}

{{$error nofilter}}

@@ -59,7 +59,9 @@ {{foreach $data as $row}} - {{$row->date}}