Adopt Eloquent model toolset for relationship tables

[asmecher]

Motivation

Currently we don't have a good, consistent, modern pattern for implementing relationship tables in OJS/OMP/OPS.

Relationship tables are tables that contain foreign keys to relate to 2 or more other tables and do not have their own autoincrementing primary keys (yet). Here are some simple examples:

• user_user_groups (relates users to user_groups)
• email_log_users (relates email_logto users)
• publication_categories (relates publications to categories)
• query_participants (relates queries to users)
• review_files (relates review_assignments to submission_files)
• user_interests (relates users to controlled_vocab_entries)

There are some others that probably qualify but are somewhat unusual:

• submission_file_revisions (relates more than 2 tables)
• subeditor_submission_group (relates more than 2 tables; kind of has its own DAO: classes/context/SubEditorsDAO.inc.php)
• submission_search_object_keywords (includes a "position" column)
• user_group_stage (relates more than 2 tables)

Currently these are implemented by jamming the functions creating/removing/updating their entries into the DAO class of one of the "parent" entities. For example, functions maintaining publication_categories entries are in lib/pkp/classes/category/DAO.inc.php; this is arbitrary as they could just as well be in lib/pkp/classes/publication/DAO.inc.php.

Goals

I propose that we...

• Begin more comprehensive use of Laravel's Eloquent ORM in a low-risk way. (We are currently only using Eloquent for lib/pkp/Domains/Jobs/FailedJob.php and lib/pkp/Domains/Jobs/Job.php.) This may prove whether/how to adopt it further in the future.
• Separate the management of relationships into their own classes, rather than putting them into one of the related entities.
• Try to keep the code for each of these as small as practical.

Proposal

Use Eloquent models to represent/maintain relationship tables.

Example model class for user_user_groups:

<?php
namespace PKP\user;

use Illuminate\Database\Eloquent\Casts\Attribute;

class UserUserGroup extends \Illuminate\Database\Eloquent\Model
{
    //
    // The following attributes configure Eloquent's expectations for this model
    //
    public $timestamps = false;
    public $incrementing = false;
    protected $primaryKey = null;
    protected $fillable = ['userGroupId', 'userId'];

    //
    // The following two functions allow callers to get User and UserGroup objects from this model by
    // accessing the ->user and ->userGroup attributes
    //
    public function user(): Attribute
    {
        return Attribute::make(
            get: fn($value, $attributes) => \APP\facades\Repo::user()->get($attributes['user_id']),
            set: fn($value) => $value->getId()
        );
    }

    public function userGroup(): Attribute
    {
        return Attribute::make(
            get: fn($value, $attributes) => \PKP\db\DAORegistry::getDAO('UserGroupDAO')->getById($attributes['user_group_id']),
            set: fn($value) => $value->getId()
        );
    }

    //
    // The following two functions permit lowerCamelCase accessors in place of snake_case column names
    // (e.g. $userUserGroup->userGroupId rather than $userUserGroup->user_group_id)
    //
    public function userId(): Attribute
    {
        return Attribute::make(
            get: fn($user, $attributes) => $attributes['user_id'],
            set: fn($value) => ['user_id' => $value]
        );
    }

    public function userGroupId(): Attribute
    {
        return Attribute::make(
            get: fn($user, $attributes) => $attributes['user_group_id'],
            set: fn($value) => ['user_group_id' => $value]
        );
    }

    //
    // The following two functions allow for queries to be executed that don't require the database column
    // names to be exposed in calling code (as ->where would do). Called e.g.: UserUserGroup::withUserId(123)
    //
    public function scopeWithUserId(Builder $query, int $userId): Builder
    {
        return $query->where('user_id', $userId);
    }

    public function scopeWithUserGroupId(Builder $query, int $userGroupId): Builder
    {
        return $query->where('user_group_id', $userGroupId);
    }
}

Calling code

Queries

foreach (UserUserGroup::withUserId(123) as $userUserGroup) {
    // This won't incur extra queries
    $userGroupId = $userUserGroup->userGroupId;

    // This will call on the UserGroupDAO to instantiate objects, so will incur extra queries
    $userGroup = $userUserGroup->userGroup;
}

Creation

Insert a new row (and return the model object):

$userUserGroup = UserUserGroup::create(['userId' => 345, 'userGroupId' => 567]);

Single entry update

// Either of the following will work
$userUserGroup->userId = $user->getId();
$userUserGroup->user = $user;

// Persist the change to the database table
$userUserGroup->save();

Batch update

UserUserGroup::withUserId(123)->update('user_id' => 234);

🛑 This is a standard formulation but requires calling code to know column names!

We rarely use this kind of query, but when we have to, a local scope containing an update call can be used to avoid exposing column names. See e.g. this Medium article for an example.

Pros/Cons

Pros:

• This is bog-standard Laravel/Eloquent code
• Plays nice with our old DAOs and EntityDAO-based implementations
• Forward compatible with any future Eloquent adoption -- we'll gain a bunch of things if and when we do
• Doesn't require a separate Collector or Repository class for simple cases like this

Cons:

• Another refactor. :( But it'll reduce the amount of old DAO code we have lying around.
• I'm not a huuuuge fan of magic function naming. Laravel seems to rely on it a lot.

[NateWr]

Separate the management of relationships into their own classes, rather than putting them into one of the related entities.

It looks like you've given this namespace PKP\user. So is the idea that each of these classes would live in the folder of one of its related entities? Or should they have their own namespace, namespace PKP\userUserGroup?

[asmecher]

Relationship entities are designed to be one class per table, so making a folder for each seems redundant. Actually I think the problem is that the User and UserGroup classes are not organized sensibly; if they were, we could have:

• classes/account/user/DAO.php
• classes/account/userGroup/DAO.php
• classes/account/UserUserGroup.php

...but I don't recommend that we rearrange these just for the sake of the relationship table, so PKP\user is unfortunately an arbitrary pick.

[NateWr]

Am I reading that right that scopeWithUserId() leads to a function called withUserId()? Do we get code completion and type hinting when we call something like the following?

UserUserGroup::withUserId(123);

[asmecher]

There's a quick video demo of the above tool with PHPStorm, and the repo mentions Sublime Text support too; hopefully it's possible with VSCode and others as well!

[asmecher]

...in fact, there does appear to be a plugin integrating this into VSCode: https://marketplace.visualstudio.com/items?itemName=georgykurian.laravel-ide-helper

[NateWr]

I think this only covers stock methods in Laravel. If we create a custom method, scopeWithSomethingCustom(), I'm not sure how the editor would pick it up.

Laravel tends to use a big docblock on facades. @jonasraoni used this well in the Locale facade: https://github.com/pkp/pkp-lib/blob/main/classes/facades/Locale.inc.php#L20-L44. But I think it will be up to us to write and maintain those code blocks for any custom methods.

[asmecher]

It does seem to cover scope functions -- see: https://github.com/barryvdh/laravel-ide-helper/blob/6e26f22c761542e6f2cdcfbe9e936e79a97d1d56/src/Console/ModelsCommand.php#L619

This test covers it: https://github.com/barryvdh/laravel-ide-helper/blob/master/tests/Console/ModelsCommand/Variadic/Models/Simple.php
The test run results in the following annotated class being generated: https://github.com/barryvdh/laravel-ide-helper/blob/master/tests/Console/ModelsCommand/Variadic/__snapshots__/Test__test__1.php

[NateWr]

Wow, cool!

[NateWr]

Forward compatible with any future Eloquent adoption -- we'll gain a bunch of things if and when we do

Can you give some examples? I think the killer feature here is eager loading, but looking at the code above it's not clear to me how that would work if the getter methods always call the Repo.

Assuming that User and UserGroup are Eloquent models, how would I grab the User and get their UserGroups at the same time without adding DB calls?

[asmecher]

The benefits would start to show when the entities on either side of the relationship table adopt Eloquent. At that point, the relationships would be defined using Eloquent syntax.

In the UserGroup model:

function user() {
    return $this->belongsToMany(User::class)->using(UserUserGroup::class);
}

In the User model:

function userGroup() {
    return $this->belongsToMany(UserGroup::class)->using(UserUserGroup::class);
}

(We might actually get rid of the UserUserGroup model entirely at this point, unless it was providing other useful functions. Eloquent does not require a relationship model to be manually specified.)

Declaring relationships this way gives a ton of functionality to models, such as tools for eager loading. You could, to make up an example...

$users = User::doesntHave('queries') // Querying on absence of relationship: Find all users without associated queries
  ->with('userGroup'); // Eager load the associated user groups

This relies on relationships being specified with Eloquent in several ways:

• Querying on absence of relationship
• Eager loading a related entity

The above syntax lets calling code choose whether or not to use eager loading. It can be specified in array form to allow nested eager loading (e.g. if you wanted to in turn fetch dependent entities on user groups).

(I haven't worked with any of these forms yet in practice, so don't know much about the particulars!)

[NateWr]

Ok thanks, that code example is useful. So the getters and setters go away once we've adopted Eloquent for all the entities. 👍

[henriqueramos]

@asmecher Why not fill the property $table on Eloquent models? Thus avoiding magic naming for tables. Also, you will be able to use the qualifyColumn across codebase.