Refactoring Sprunje Class

[lcharette]

Tasks

• [x] Refactor constructor (see below)
• [ ] Add built-in caching (at least for base query; to the whole result if possible)
• [x] Add properties feature

---

Current Sprunje Constructor is too complex. The problem is the query method is called inside the constructor. So if the query requires more info (eg. $foo relation model), you can't do this :

// Controller
$sprunje = new BarSprunje($classMapper, $params);
$sprunje->setFoo($foo);

// BarSprunje
protected function baseQuery()
{
    return $this->foo->bar();
}

Instead the ctor needs to be extended ($sprunje = new BarSprunje($classMapper, $params, $foo);), which in the long term can create issue if a new argument needs to be added to the core class.

Same goes if any additional services are required by the Sprunje and/or transform method.

[alexweissman]

What do you propose?

In my Sprunjes, I've been creating a new method that allows me to access the underlying query object:

// Controller
$sprunje = (new StudentSprunje($classMapper, $params))->forWorker($worker);

// StudentSprunje
public function forWorker($worker)
{
    $this->query = $worker->currentStudents();

    return $this;
}

That being said, I've run into other issues with the Sprunje API that could indicate a need to refactor/rework it.

[lcharette]

Your solution could also work in my case. But doesn't it makes the baseQuery abstract method useless? extendQuery can be useful when adding stuff to the base query, but not much setting the base relation...

Also, if you need to inject another service (Router, cache, etc.) to use with Transform for example, it won't be much of help.

And speaking of refactoring, caching is another issue when dealing with Sprunje...

[alexweissman]

Yes, I agree, I'm starting to see a lot of structural issues with Sprunje. Maybe we should add this to the UF5 roadmap? I also want to add a properties feature to Sprunje, so that you can do something like this:

GET http://localhost/btoms/public/api/workers?size=10&page=0&sorts[info]=desc&properties[]=lastActivity&properties[]=institutions&properties[]=phones

This allows you to specify which relationships (and even direct properties) of a model to load. Maybe every Sprunje could have a set of default properties to load, and then additional properties could be specified in the parameters.

It would have to be integrated with our access control system somehow, I suppose.

[lcharette]

Ok. Let's make this issue the Sprunje refactoring issue.

[lcharette]

I've done part of the refactor in https://github.com/userfrosting/sprinkle-core/commit/526bb8e8aa15c6e5bfdcf9363c907ad12ed6d7bb.

The constructor have been simplified and an alternative to the option in the contractor have been added (sprunje::setOptions($options)). While custom sprunje will still require to extend the constructor to add new dependencies, modern dependency injection (PHP-DI) bypass the issue originally described, as long as options are not passed in the constructor. In essence:

• Before :

public function getList(Request $request, Response $response)
{
    /** @var \UserFrosting\Sprinkle\Core\Util\ClassMapper $classMapper */
    $classMapper = $this->ci->classMapper;

    $sprunje = $classMapper->createInstance('activity_sprunje', $classMapper, $params);
    
    return $sprunje->toResponse($response);
}

• After :

public function getList(Request $request, Response $response, ActivitySprunje $sprunje)
{
    $sprunje->setOptions($params);
    
    return $sprunje->toResponse($response);
}

[lcharette]

I also want to add a properties feature to Sprunje

Done in https://github.com/userfrosting/sprinkle-core/commit/7ce3e0396991738eb41836b73fb4d9f2fd500626 ! List of columns to show added as property (and not options; as theses proves difficult to default, and array are bad at type check).

[lcharette]

As far as further rewrite goes, I propose to separate Sprunje between data definition (what we want to get) and what return the data :

class Sprunjer implement Sprunjer {
    public function __construct(SprunjeInterface $sprunje) { ... }
    public function toArray() { ... }
    public function getListable() { ... }
    //  etc.
}

class Sprunje implement SprunjeInterface, SortableSprunjeInterface, ListableSprunjeInterface, FilterableSprunjeInterface {
    protected array $filterable = [];
    protected array $listable = [];
    protected array $sortable = [];
    protected function baseQuery()
    // etc.
}

Big pro is Sprunjer can be extended : CsvSprunjer, ResponseSprunje, ExcelSprunjer, etc... One could add a feature globally to all Sprunje in a sprinkle (not possible right now), like caching. It will also enabled to better separate thing and declutter curent Sprunje class.

Sprunje (another name could be used to avoid confusion) itself could implement variable feature thought implemented interface. Sprunjer could check if Sprunje implement SortableSprunjeInterface before applying sort for example.

Finally the array of 'options' should be replaced with specific property (because array based options are bad at type check).

---

I'll probably target 5.1 for this fix, therefor create a new issue and move caching feature there.