search

mrenigma / bolt

Bolt is a simple CMS written in PHP. It is based on Silex and Symfony components, uses Twig and either SQLite, MySQL or PostgreSQL.
MIT License
0 stars 0 forks source link

Gawain Feedback #4

Open graham73may opened 7 years ago

graham73may commented 7 years ago

OK, first comment … the traits in Storage should never be looked at for approach, they were a very ugly compromise, hence the:

 * These traits should be considered transitional, the functionality in the
 * process of refactor, and not representative of a valid approach.

[7:32] Also, no snake_case variables/methods in PSR-2

graham73may commented 7 years ago

Things like $this->app['storage'] should never appear in a trait, you should have an abstract method like abstract function getStorage(); so that users of the trait are forced to implement, and ensure, that said service is available Basically a trait should always be "self contained"

I think there are service helper methods in Base & BackendBase that covers most/all of the services in that trait, so easy update empty() needs to go, check types and be strict

graham73may commented 7 years ago

Also, in a few days that is going to fail Travis on a few style violations… The master branch on https://github.com/bolt/codingstyle is a good place to start I'm a huge offender on this historically, but the $foo = $bar alignments are going to be one of those fails

As for instantiation … A \Pimple service in the SP, as it is (mostly) used in the controller, they all have $app so a no brainer … When you're closer to done we'll pull Ross in as well … Storage is his bambino

The link() thing has me wondering though … this would likely be aiming at v3.5 so maybe it is time to finally kill it (link()) somehow (edited)

Also for everyone's sanity, especially your own … I'd put your work in a feature/hierarchical-content branch based on 3.x org branch

graham73may commented 7 years ago

mrenigma [8:03 AM] Ha! Should we target v3.5 or v4?

gawainlynch [8:03 AM] 3.5 (3.x branch) as v4 (master) is still 6+ months away

mrenigma [8:04 AM] Soooooo 3.5 it is