Refactored the Grav classes load and process methods to follow clean coding standards #745
Conversation
to follow clean coding standards. Cleaned up container setup Added comments for the new methods in Grav Removed not needed use statements from grav file Refactored the grav->process method to be more extendable Cleaned up a bit, reordered class methods, used measureTime in load
| $this->measureTime($processor->id, $processor->title, function($debugger) use ($processor) { | ||
| $processor->process($debugger); | ||
| }); | ||
| endforeach; |
There was a problem hiding this comment.
We aren't in a template file here, please use { ... } instead. :)
There was a problem hiding this comment.
As the other code uses the classical notation { ... } I have changed it. But I've not see any guideline that say the alternative syntax should only be used in templates. Happy to learn something. :)
| // then to get it from the container all time. | ||
| $container->measureTime = function($timerId, $timerTitle, $callback) use ($debugger) { | ||
| $debugger->startTimer($timerId, $timerTitle); | ||
| $callback($debugger); |
There was a problem hiding this comment.
I do not like having $debugger as a parameter. The parameter seems to have no purpose, which is why I ended up using a lot of time trying to understand reasoning behind it. I see it being used in 2 processors out of 13, but then again, its already available through the container. If its just there because of optimization, there are better ways to optimize code.
There was a problem hiding this comment.
You've got a point. I'll change that.
…as it is already injected in the container
… container already exists
| * | ||
| * @return void | ||
| */ | ||
| protected function registerServices($container) { |
There was a problem hiding this comment.
$container = $this :) I don't think the parameter is needed anymore.
…n thus does not need to be passed through any more
|
I cannot find anything else to be fixed. :) I like the change, even if right now there are not real benefits of having the array except the code refactor to make it more abstract. I'm thinking if we should allow plugins to use the new protected methods you added.. Hmm.. Also at some point we might want to allow |
|
You are right, the changes will make the system more open in future, there is no immediate benefit that e.g. a feature adds. I'm off the opinion that the core also needs refactorings like this one, else Grav might end up with a codebase like other successful CMS that are nearly unmaintainable cogh Wordpress cogh. ;) |
|
There are multiple classes in Grav that I would refactor right away if I had the time to do it. One of the most painful ones are Page and Pages classes, which have experienced the most changes since we first implemented them. I'm just afraid the changes I would make would also break B/C, so they just need to wait a bit longer. Changes like this one are really welcome. Still I'd like to wait to have extra pairs of eyes checking out this code and the performance implications it may have (I doubt there are any, but its something we also need to consider). @rhukster I know you're at your vacation, but if you have some extra time, this will be one of the most interesting things you could look into. :) |
|
@mahagr sure, that all makes sense. As we might use Grav for more clients we also might investigate even more. |
|
Oh I see what you mean, 2526 locs... |
|
I really like the ideas behind this refactor. Definitely does improve readability. I can see that down the line having more public API methods to allow for plugins to interact directly with the processors could be very useful. I only have a couple of days left on my vaca and i'll be back home. I'll look at this more thoroughly this weekend. Thanks! |
|
I know we've made a few changes that is causing some conflicts now. Would you mind updating this PR with the latest changes in the develop branch and i'll do some testing? Thanks!!! |
|
@rhukster I'll try to look into the conflicts next week |
|
@rhukster merged, tests are ok (except the 5 that did go wrong before) and the demo system runs. |
|
Ok, i've tested on my local and everything seems to be fine. Performance differences are negligible which was my only real concern. Thanks for your hard work on this, i think it's going to make Grav very flexible going forward. |
|
Happy to help :) |
|
P.S. tests should be good now. |
When we evaluated Grav the first class I've looked into was Grav/Common/Grav. I loved the container and the central process chain, but the code seemed to be "copy & paste". Nearly the same code was repeated over and over again. The open-closed-principle was not applied. I've refactored the process and load method using two arrays as basis to load the needed services dynamically into the container and to use them in the process method in a more generic way. New services and processes can now be added without modifying existing mode (only the arrays would need to be modified).