Bolt 2.x Refactoring

Areas

• Storage
• Content
• Tests
• Exceptions
• Service Providers
• Controllers / Twig
• API
• The Field Database Problem
• Symfony Forms & validation in the backend
• DBAL Queries and QueryBuilder
• Extensions

General

• Add nn experimental: false|true field in config.yml so that we can add extra features without impacting base users and BC
• Gradually move away from using file_exists, is_writable, is_readable etc. and start using the Filesystem object to centralise these checks. This will help a lot in increasing code coverage since we can mock the Filesystem object whereas mocking native PHP functions is extremely difficult.

Storage

• To ORM or not to ORM?
• Doctrine ORM adds 3.4 MiB to vendor/
• I still like Ross' approach of having the enablers in core, but the implementation in Extension Land — Gawain
• 1 database table to 1 Entity class setup
  • I think there are some exceptions though. Authtoken might have it’s own Entity but not its own Repo.
• Separate the fetches (sql queries) into their own Repo classes
• Have only a single way for Bolt to get to the database connection
  • hopefully reduce the security footprint
  • Would this mean disallowing access via $app[‘db’] ? I think that’s not a good idea, because people (extension devs) would then just instantiate their own DB objects, making it messier.
• Example: Bolt\Entities\User class (pseudo code below)
  • would be the one specific to managing User entities, not something that might go to a service provider
• Entities are supposed to be plain PHP objects that just manage the data for a user
  • I think it’s ok for them to proxy calls to a repository though (see lazy load below)
• Bolt\Entities\User is the data specifically
• When you fetch a user from the db you will get a user entity object back,
• So you can do:
  • $user->getId()
  • $user->id if we implement auto getters
• Bolt\Storage becomes BoltEntityManager....
  • It acts as a single interface between bolt and the db...
  • That and only that
  • At the moment it does everything, but we can gradually migrate it's responsibilities away
• For instance: getContentObject()
  • we could just tackle content first, and then proxy that method to the new one
  • then we can move onto the next one etc....
  • Hence the strategy with the tests
• Lazy load Taxonomy and Relations for Content
  • I think this means passing a TaxonomyRepository to the Entity so it can call it with itself as parameter to the repo to fetch taxonomies and store it with itself.
• Start throwing deprecated errors on anything that will disappear in 3.0
  • We should definitely do this for magic methods that maintain BC

Storage Setup Example:

e.g. src/Entities/User.php:

<?php

namespace Bolt\Entities;

class User extends BoltEntity {
    
    public $id;
    public $username;
    public $password;
    public $email;
    public $lastseen;
    public $lastip;
    public $displayname;
    public $enabled;
    public $stack;
    public $roles;
    
}

class UserRepository extends BoltRepository  
{
    public $entityManager;
}

class BoltEntityManager 
{
    public $connection;
}

e.g. in src/Application.php:

$users = $this->entityManager->getRepo('Users');
$user = $users->find(['id'=>5]);
$allusers = $users->findAll(['enabled'=>1]);

Content

• ContentType interface
• ContentType …meets Entity
• Phase 1 needs magic:
  • which will need to be provided by the EntityManager…
  • when someone adds a Contenttype it needs to be read via the config, and Bolt will need to use that data to return an auto generated object
• Create classes for config objects:
  • TaxonomyType
  • ContentType
    • Field (list)
      • FieldType (currently \Bolt\Field\FieldInterface)
  • These would have ArrayAccess / magic to be BC.
  • I don’t think these need interfaces since the are just containers (with the exception of FieldType), but I could go either way.
  • One of the goals is to reduce the code in Config, so where should the array be parsed into these objects? Currently I have ContentType::fromConfig which creates a instance and parses the array into it.
  • I know Ross hates statics, so I’m open to suggestions. Another option is a factory class.
  • They need to be serializable too. Unserialization should skip the parsing checks.

Tests

Combine test/ and tests/ into:

• tests/unit/
• tests/acceptance
• …or similar

Exceptions

There’s a lot all under LowlevelException along with static methods.

It would be nicer if we used PHP exception objects and made them more specific, eg

throw new UnsupportedDatabaseException('message', 'code'); 

…rather than the current:

throw LowLevelDatabaseException::unsupportedDriver($driver);

The above will make testing a bit more straightforward since we can test that a specific Exception class is thrown, rather than having to parse the message.

Look at organisation too, it would possibly be nicer if exceptions sat under src/<Module>/Exception/ExampleException, rather than being in a top level Exceptions namespace.

Service Providers

The aim of Service Providers should be to inject the necessary dependencies into the classes they provide, at the moment the default is to pass the entire Bolt app into each of these dependencies.

So for instance, this is the correct approach: PrefillServiceProvider The prefill class only needs a guzzle client and the provider wires this up. That means we can write tests for the Prefill class and never need to know that there is a Bolt app sitting above everything.

Obviously this is a harder change for some classes since access to the $app is tightly woven throughout, but in these cases it may be a sign that a refactor is needed.

Another option as a stepping stone to this, would be to assign the class variables from the app in the constructor instead of passing them in. That way all uses in the class can be updated and then the constructor’s signature can change later (I’m not sure how concerned we are with BC)

Easy targets:

• Cron.php: pass in the dispatcher as a dependency
• StatService: pass in the logger and guzzle dependencies
• OmniSearch: mainly needs a URL generator service passed in.

Controllers / Twig

There is a lot of similar functionality in our controllers. I think we need to create a BaseController that has shortcuts for these methods. Symfony has this here. I imagine ours being really similar.

Backend controller should be split up more.

• Log Controller
  • systemLog
  • changeLog
  • changelogRecordAll
  • changelogRecordSingle
• Users
  • list
  • userEdit
  • userFirst
  • roles
  • profile
  • userAction
• Files
  • files
  • fileEdit
• TwigResponse class
  • Stores template name and variables
  • Allows after handlers to access/modify variables
  • Controllers should return these instead of rendering html strings
  • These responses need to rendered to html at some point in after event priority
• Twig files should be moved into a subfolder (within the root twig folder) matching the name of their controller.
• Twig files should be namespaced with their controller
  • Perhaps the current root twig folder can be have a namespace @bolt and common files could stay there.

API

We need to make more of an effort to define this. Especially since bolt can be pulled in as a composer dependency now. It’s a lot easier to extend core classes and then have problems on updates.

I think we should define classes and methods individually with an "@api" annotation. I know there is a lot in flux right now so this allows us to define certain parts when we are ready.

Symfony does this too :)

Field Database Table Problem

At the moment we are starting to have a few examples of fields that store serialized JSON as their values. Whilst it isn't an immediate problem since the current use-cases are simple we have a lot of requests for adding more complex field types and we could get to a situation, if we continue using json, where a lot of data is stored non-natively in the database.

Everything below works on the concept of leaving basic fields untouched in their current behaviour, there may be several of the current fields that could be migrated to using field-specific storage but this would be optional.

Since we now have separate classes for each type of field we could hand over persistence responsibility to the individual field classes to describe how they should be persisted to the database.

For instance all the basic fields already specify their storage type via: getStorageType() and return text

Bob notes: If we do this, we'll break away from the "one Contenttype, one table" structure that we've had. Not saying this is bad necessarily, just putting it out there.

Gawain notes: Redistributes load to a more appropriate target, JSON decoding isn't overly expensive in most use-cases, but nonetheless there is a small win here

Suggestions: Start supporting an additional storage type of ‘field’, along with an array of options that can provide extra meta information.

We add two an extra field tables to the database.

bolt_field
id    contenttype    contenttype_id    field     field_value_type

bolt_field_value
id	field_id		field_value

This has the added benefit of immediately supporting multiple values so for instance an imagelist would be stored as multiple rows in the database eg:

bolt_pages:
id:1
imagelist: 1 - //since imagelist would be configured as field storage, the fetch would know to take the following route:

bolt_field
id:1, contenttype:pages, contenttypeid:1 field:imagelist type:varchar

bolt_field_value
id:1, field_id:1, field_value: /path/to/image/1.jpg
id:2, field_id:1, field_value: /path/to/image/2.jpg
id:3, field_id:1, field_value: /path/to/image/3.jpg

Ordering of multiple field relations

The very first question from users with multiple fields is always: "I want this item before that item, how can I do that?"

The simplest solution would be adding a "weight" field to the relation what will be used in the queries for sorting.

bolt_field_value
id:1, field_id:1, weight:1, field_value: /path/to/image/1.jpg
id:2, field_id:1, weight:3, field_value: /path/to/image/2.jpg
id:3, field_id:1, weight:2, field_value: /path/to/image/3.jpg

• A drag and drop interface would be a solution for the editors - which needs a little javascript magic.
• The initial values are always complicated too, if you start with everything null or uninitialized, the values need to be filled in as soon as one item is reordered.

Supporting multiple types

In the future we could maybe add support for multiple storage types and then we would need to add additional tables eg:

bolt_field_value_float
id	field_id		field_value

bolt_field_value_int
id	field_id		field_value

In the above cases the relevant table would be automatically selected based on the field config.

Symfony Forms & validation in the backend

Create a forms sub-namespace and push our form generation and validation to there

• Use classes instead of arrays
• Validation can be assigned inside of class
• Appropriate fields types should be created. The one that comes to mind right now is yaml. The yaml validation automatically be assigned to this form type. So you just say this is a yaml field it will validate the contents when validating form.
• Input::cleanPostedData could be integrated too.
• Fuzzy on the details, but this is the general idea.

DBAL Queries and QueryBuilder

Standardise our DBAL querieson \Doctrine\DBAL\Query\QueryBuilder.

An (over done) example of this would be the query:

$query = sprintf(
    "SELECT id, slug, title, COUNT(id) AS count 
        FROM bolt_entries AS entries
        WHERE entries.ownerid = %s
        GROUP BY status
        ORDER BY datepublished DESC
        LIMIT %s", 
    $owner, $limit);

$app['db']->fetchAll($query);

Would become:

$query = $app['db']->createQueryBuilder()
    ->select('id, slug, title, COUNT(id) AS count')
    ->from('bolt_entries', 'entries')
    ->where('entries.ownerid = :ownerid')
    ->groupBy('status')
    ->orderBy('datepublished', 'DESC')
    ->setMaxResults($limit)
    ->setParameters(array(
        ':ownerid' => $owner)
    ));

$results = $query->execute()->fetchAll();

Extensions

We need to split Extension::initialize method up.

1. We should have a "register" and "boot" methods just like service providers.
2. We should emit new events for "backend.before/after" and "frontend.before/after" that ppl can hook into do some of the permissions logic and stuff that needs request/user info

Also, on a similar note. I think we need to autoload the extensions earlier in our Application::init process. (That doesn't include initializing them though)

Basically register() should not invoke any of the stack. Only extend services / register new ones. Boot() should be for things like adding listeners/subscribers to the dispatcher