ThemeData attachments should rely on attachMany relationship instead of multiple attachOne

[RomainMazB]

Hi.

As I've already talked with @LukeTowers on slack, I've "discovered" that the ThemeData model is using multiple attachOne relationship as you can see here to handle the files upload you may need to customize your theme.

The problem of that is when you create a theme as I did which use multiple file upload fields to be customized: displaying a page just blow your database.

As an example: I've created a theme which was making 32 queries on home page :o without any possibility to eager load all theses queries due to the fact they are all distinct with the attachOne relationships.

I've finally succeed to change this behaviour locally in the project I'm working on, but since it ended in a huge performance increase, I wanted to discuss about that.

What was the performance gain? I've ended with only 9 queries on my home page. To remind you: I've began at 32 so I've divided the queries amount by more than 3 (and actually the assets queries from 24 to only 1, so the assets queries are divided by 24 in my theme!)

How I did is here in a plugin boot method:

ThemeData::extend(static function ($model) {
    $model->bindEvent('model.afterFetch', static function () use ($model) {
        $model->afterFetch();

        // If attachments exists, override their accesses
        if (! blank($model->attachOne)) {
            // Create an attachments hasMany relationship
            // I could have done this using $attachments = File::where(...) but relationship is cleaner IMO
            $model->addDynamicMethod(
                'attachments',
                static function () use ($model) {
                    return $model
                        ->hasMany(File::class, 'attachment_id')
                        ->whereAttachmentType(ThemeData::class);
                }
            );

            // Load all attachments with only one query
            $attachments = $model->attachments()->get();
            foreach ($model->attachOne as $field => $value) {
                // Override properties defined with attachOne relationship
                $model->addDynamicProperty(
                    $field,
                    $attachments->firstWhere('field', $field)
                );
            }
        }
    });
});

Since I didn't remove the original attachOne relationships, I doesn't create any issue.

When I've done that I said to myself: you have to implement it in the core and make a PR! I've thought about it and said to myself it wouldn't be a breaking change because all accesses are unchanged: you still access your data as before using $theme->getCustomData()->myAttachmentFieldName and you can still attach/detach models.

But the way I've done it eager-load all the attachments, even if they are not needed, which was not the case previously and said to myself it could be problematic in some way?

On the other hand I'm saying to myself: if a page is requested, the theme will be loaded and there is already probably at least one file your theme will look into the database to display - as an example: the logo. So you are basically making a query to load it on each page, this enhancement will provide all your assets with this little query instead of querying a second (and third, and fourth, ...) time if you need other assets on this page.

Since I'm verifying that $attachOne is not blank before doing all this stuff, this enhancement will not create any performance issue if the theme is not using any file upload - probably more 90% of the theme you will ever create. This only provide a performance 🚀 boost 🚀 to those which use file uploads.

Any thought about that?

[LukeTowers]

So basically your proposal is to continue having an attachOne relationship defined for every fileupload field used on a theme, but behind the scenes have an attachMany relationship that retrieves all of the related files in one query and sets the value of all the attachOnes to their appropriate value?

[RomainMazB]

@LukeTowers

Yep, I said to myself we maybe should keep the attachOne to prevent any breaking change I can't see. We could find a way to delete it without any changes in the way we attach/detach or access it.

But maybe we should keep it, just in case where someone used $theme->attachOne to do whatever with the attachments list.

[github-actions]

This issue will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this issue is still relevant or you would like to see it actioned, please respond and we will re-open this issue.
If this issue is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

[github-actions]

This issue will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this issue is still relevant or you would like to see it actioned, please respond and we will re-open this issue.
If this issue is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

[github-actions]

This issue will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this issue is still relevant or you would like to see it actioned, please respond and we will re-open this issue.
If this issue is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

[github-actions]

This issue will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this issue is still relevant or you would like to see it actioned, please respond and we will re-open this issue.
If this issue is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

[daftspunk]

Hey @RomainMazB, thanks for this. I have made a note in our internal tracker that the N+1 issue exists for theme data. The fix is simple but will require some testing efforts to ensure the fix is adequate

It looks like you have a good workaround in place in the meantime. Nice solution! 🦾