Code review and suggested updates

pamtbaau commented 3 years ago

I just couldn't resist to have a look at this plugin too...

I've make a couple of changes:

Improved the use of composer autoload
Moved all data logic into /classes/ipcounter.php
Used simplified registration of Twig function instead of extension.
Moves loading assets from twig template into ipcount.php.
No need to copy visitor.html.twig into theme. Template is registered by plugin.
Page visitor.md with statistics does not need any settings.
Rewritten js file:
- Removed repeating async server calls and reuse data provided by server
- Chart library Plotly is quite heavy (~3.4MB). Replaced it with lightweight library Chart.js (~178 Kb)

Did I introduce issues? Probably... Is there room for improvement? It sure has... Could a breaking change be introduced? Maybe... Does it run on my machine (Windows 10 + WSL 2) using Chrome? Yes.

How to test? git clone https://github.com/pamtbaau/grav-plugin-ipcount.git . -b develop into /user/plugins/ipcount and it should work.

Todo:

Store data in a more structure way. This will allow the frontend to pick the year/month in one go without looping over all 'days' entries in current json format.
```
{
"count": 123456,
"2101": {    <-- year/month
  "01": 324,   <-- days
  "02": 123
}
} 
```

wernerjoss commented 3 years ago

I just couldn't resist to have a look at this plugin too...

:smile:

I've make a couple of changes:

* Improved the use of composer autoload

* Moved all data logic into `/classes/ipcounter.php`

* Used simplified registration of Twig `function` instead of `extension`.

* Moves loading assets from twig template into `ipcount.php`.

* No need to copy `visitor.html.twig` into theme. Template is registered by plugin.

* Page `visitor.md` with statistics does not need any settings.

* Rewritten js file:

  * Removed repeating async server calls and reuse data provided by server
  * Chart library Plotly is quite heavy (~3.4MB). Replaced it with lightweight library Chart.js (~178 Kb)

looks all ok. and yes, Plotly is heavy - I used it befor for other, more complicated graphics, so I stuck with it here, too. but, Chart.js is much more lightweight and does this job here well.

Did I introduce issues? Probably...

I checked it already on my live site and it works ok there. however, I want to check some mor if everything is ok before making a new release to the public.

Is there room for improvement? It sure has... Could a breaking change be introduced? Maybe... Does it run on my machine (Windows 10 + WSL 2) using Chrome? Yes.

runs here on debian buster, too.

How to test? git clone https://github.com/pamtbaau/grav-plugin-ipcount.git . -b develop into /user/plugins/ipcount and it should work.

Todo:
* Store data in a more structure way.  This will allow the frontend to pick the year/month in one go without looping over all 'days' entries in current json format.

this is a point I already had thought about too and my current workaround is the helper-script CountRotate.php which can be called manually or by cron. it will preserve historic data but limit the file counter.json to a predefined time span so it will not take too long to parse.

{
   "count": 123456,
   "2101": {    <-- year/month
      "01": 324,   <-- days
      "02": 123
   }
}

that would be ok, I think. the problem here is, that grav does not provide a hook that can be used to do e.g. data format migration upon plugin update, one of the few things I like more on Wordpress, which provides such a feature. or did I just miss something here ?

pamtbaau commented 3 years ago

grav does not provide a hook that can be used to do e.g. data format migration upon plugin update

You might try the following:

Just before saving the data, you can check if the current content of the file contains the old structure:

if (isset($data['days']) {
$newDataFormat = $this->migrateData($data);
}
$newDataFormat[$year]['$today'] = ....

Maybe data structure should be:

"count": 32466,
"years": {
   "2021": {
      "months": {
         "01": {
            "days": [
                "01": 23
            ]
        }
     }
   }
}

A bit more verbose, but cleaner.

wernerjoss commented 3 years ago

grav does not provide a hook that can be used to do e.g. data format migration upon plugin update

You might try the following:
* Just before saving the data, you can check if the current content of the file contains the old structure:

yes, I already thought about such a procedure - but frankly, I don't like this really, introducing a check which will be useless in 99.9% during normal operation.
I would much more appreciate having something like an event onPluginsUpdate in Grav, see this issue

pamtbaau commented 3 years ago

Well, how expensive can a simple if (isset($data['days']) be, compared to the total processing cost of the entire plugin?

wernerjoss / grav-plugin-ipcount

Code review and suggested updates #2