Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only retrieve forms once instead of for every item #330

Merged
merged 4 commits into from
Jul 31, 2024
Merged

Only retrieve forms once instead of for every item #330

merged 4 commits into from
Jul 31, 2024

Conversation

dnwjn
Copy link
Contributor

@dnwjn dnwjn commented Jul 31, 2024

Hello everyone!

We stumbled upon a bug on /admin/forms. We would get endless loading screens and eventually timeouts when visiting this page. We have quiet an amount of form submissions (~150.000), and the query builder in the current setup retrieves all submissions, with the form for every one of them.

We're using Statamic 5 with this package, and even though we were able to fix this in the FormsController of the statamic/cms package (see statamic/cms#10534), we figured this could still cause problems for other scenarios outside of the FormsController. So we wanted to propose it.

The proposed changes retrieve all forms only once before being connected to each submission, instead of retrieving the form every time. This drastically decreases the amount of queries being executed, and it fixes the timeout issues we were dealing with.

If you have any questions or comments about this, let me know!

@ryanmitchell
Copy link
Contributor

ryanmitchell commented Jul 31, 2024

Thanks for the PR!

I would prefer we blink the query result in the Form Repository, similar to how we do it elsewhere (eg collections -

public function all(): IlluminateCollection
{
return Blink::once('eloquent-collections', function () {
return $this->transform(app('statamic.eloquent.collections.model')::all());
});
}
public function find($handle): ?CollectionContract
{
return Blink::once("eloquent-collection-{$handle}", function () use ($handle) {
$model = app('statamic.eloquent.collections.model')::whereHandle($handle)->first();
return $model ? app(CollectionContract::class)->fromModel($model) : null;
});
}
)

Do you want to try refactoring it to take that approach and see how it works from a performance perspective on your install?

@dnwjn
Copy link
Contributor Author

dnwjn commented Jul 31, 2024

@ryanmitchell Appreciate the quick response! I've implemented Blink, and I'd say it gives about the same results (~6.5 seconds). I see how Blink is more in line with the rest of the package, so let me change that right away!

@ryanmitchell
Copy link
Contributor

I've pushed up some additions to clear the blink cache on save/delete as per the collection repository, and I've also added test coverage as we're trying to get the test suite up to a satisfactory level.

Thanks again for the PR, appreciate you taking the time to submit it.

@ryanmitchell ryanmitchell merged commit a96f861 into statamic:master Jul 31, 2024
11 checks passed
@dnwjn
Copy link
Contributor Author

dnwjn commented Jul 31, 2024

@ryanmitchell Thanks for taking care of this so quickly! If I ever create another PR I'll make sure to take testing into account as well!

@dnwjn dnwjn deleted the patch-1 branch July 31, 2024 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants