Dispatch conversion jobs after database transactions have been committed

[chrispage1]

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]

I can confirm this fixed the issue 🚀

[francoism90]

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

[chrispage1]

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]

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

[freekmurze]

Thanks!

[chrispage1]

Thanks @freekmurze ! Have a great day!

[Tahiaji]

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]

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]

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]

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

[Tahiaji]

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

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

[chrispage1]

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]

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]

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!