jonom / silverstripe-betternavigator

Front-end utility menu for SilverStripe websites featuring administration and development tools
60 stars 31 forks source link

Minor tidy & new code for flushing on dev build #2

Closed michaelbollig closed 10 years ago

michaelbollig commented 10 years ago

A few minor tidy up points, mainly:

jonom commented 10 years ago

Thanks for this Michael. Will certainly make use of the housekeeping but just want to determine the benefit of flushing templates during a build before I merge that part. Simon infers it should happen by itself almost all the time, so am seeking further clarification from him on that point. If I do merge that part I think I would also just need to semi-revert the 'dev/build' references to 'dev/build?flush' as Swaiba's code only kicks in if the flush url parameter is present.

michaelbollig commented 10 years ago

My absolute first pull request, so excuse the terribleness. :)

Regarding the need to flush with a build, the real issue here isn't templates—as Simon mentioned. Basically it's to flush the manifest. If you add in a new module, change your YAML code and do a dev build, you'd expect everything to be peechy, however without a flush the manifest and new config changes aren't included. Good point that we still need the dev/build?flush part, I hadn't read that clearly enough. I'm more than happy to not include the FlushOnDevBuild script at all, since it's not exactly core to the module's purpose, however we would find it useful so I figured most other developers would as well.

From here I need to change some things on the PR? Forgive the ignorance :) Or is it easer to just manually apply what you need?

As an aside, I think the way 3.1 flushes etc is a bit counter-intuitive. ?flush=all is almost entirely useless, but from the name and 2.4 usage, it would seem to do everyhting a ?flush does PLUS MORE. Except it doesn't, it flushes templates that don't need to be flushed 99% of the time. But I digress...

jonom commented 10 years ago

Yes, definitely agree there is a need to use dev/build?flush rather than just dev/build in order to flush the class manifest etc., but think the extra functionality from the FlushOnDevBuild class (flush templates and template manifest during a dev?flush) should be in core rather than this module. In fact it looks like it's recently been implemented (if not quite released) - nice! https://github.com/silverstripe/silverstripe-framework/commit/2b316e79e5f957bfe06385f762c237acaf75b0e7