search

spatie / laravel-medialibrary

Associate files with Eloquent models
https://spatie.be/docs/laravel-medialibrary
MIT License
5.78k stars 1.08k forks source link

Dispatch conversion jobs after database transactions have been committed #3687

Closed chrispage1 closed 3 months ago

chrispage1 commented 3 months ago

I have been running into an issue (#3677) and it has been raised previously (#3674) that media conversions sometimes immediately fail.

Having done some debugging I've found the cause to be media that is added within a database transaction. In my case this was apparent in Laravel Nova when creating a post with large media.

This means when the queue worker tries to generate the conversions, they immediately fail because the record has not yet been persisted to the database.

The fix is to ensure that these jobs are only dispatched after the data has been committed to the database. This can easily be enforced by calling ->afterCommit() on the dispatched job. If there is no active transaction the job will be dispatched in the usual way.

royduin commented 3 months ago

I can confirm this fixed the issue 🚀

francoism90 commented 3 months ago

You can set this globally: https://laravel.com/docs/11.x/queues#jobs-and-database-transactions

chrispage1 commented 3 months ago

You can set this globally: https://laravel.com/docs/11.x/queues#jobs-and-database-transactions

I did see that this was possible and mentioned it in #3674 but this PR would address the issue for anyone running this package regardless of whether the config variable is set or not. A worthy addition I think 👍🏻

chrispage1 commented 3 months ago

Hey @freekmurze - reckon this is something you could merge in? Any particular concerns? Thanks!

freekmurze commented 3 months ago

Thanks!

chrispage1 commented 3 months ago

Thanks @freekmurze ! Have a great day!

Tahiaji commented 3 months ago

This fix is ​​not very obvious, but it crashed tests on my project. Tests are wrapped in a transaction. Access to conversions is no longer available. For now, I rolled back to 11.8.2

chrispage1 commented 3 months ago

This fix is ​​not very obvious, but it crashed tests on my project. Tests are wrapped in a transaction. Access to conversions is no longer available. For now, I rolled back to 11.8.2

Any specific error you're getting conversions just aren't being shown?

What version of Laravel are you running, 10 or 11?

And how many transaction layers deep are you?

chrispage1 commented 3 months ago

This fix is ​​not very obvious, but it crashed tests on my project. Tests are wrapped in a transaction. Access to conversions is no longer available. For now, I rolled back to 11.8.2

Ah I see, your tests are wrapped in a transaction. In that case conversions wont be generated until after the transaction has completed. Any particular reason for your tests being within a transaction?

royduin commented 3 months ago

I think it's default behaviour due the RefreshDatabase trait: https://laravel.com/docs/master/database-testing#resetting-the-database-after-each-test

Tahiaji commented 3 months ago

Exactly. We use trait RefreshDatabase to significantly speed up test execution.

The change seems minor, but it breaks backwards compatibility to some extent.

chrispage1 commented 3 months ago

Created a PR #3698 - if accepted, this will allow you to optionally change the default behaviour which would be particularly useful within your tests.

I did consider just disabling this behaviour when running unit tests, but I think that could cause additional undesirable behaviour. At least this way, it'll be easy to simply turn the behaviour on/off.

Tahiaji commented 3 months ago

Created a PR #3698 - if accepted, this will allow you to optionally change the default behaviour which would be particularly useful within your tests.

I did consider just disabling this behaviour when running unit tests, but I think that could cause additional undesirable behaviour. At least this way, it'll be easy to simply turn the behaviour on/off.

Thank you very much. The ability to disable this specifically for testing purposes would be extremely useful.

chrispage1 commented 3 months ago

Created a PR #3698 - if accepted, this will allow you to optionally change the default behaviour which would be particularly useful within your tests. I did consider just disabling this behaviour when running unit tests, but I think that could cause additional undesirable behaviour. At least this way, it'll be easy to simply turn the behaviour on/off.

Thank you very much. The ability to disable this specifically for testing purposes would be extremely useful.

It looks like Freek was on the case and this has been merged!