spatie / laravel-medialibrary

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

UPGRADING sample migration produced invalid SQL #2428

Closed ethanclevenger91 closed 3 years ago

ethanclevenger91 commented 3 years ago

MariaDB: mariadb Ver 15.1 Distrib 10.5.8-MariaDB, for debian-linux-gnu (x86_64) using readline 5.2 via Laravel Homestead

When running the migration documented in the UPGRADE guide:

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '>"$.generated_conversions", `media`.`updated_at` = ? where `generated_convers...' at line 1 (SQL: update `media` set `generated_conversions` = custom_properties->"$.generated_conversions", `media`.`updated_at` = 2021-05-06 22:35:58 where `generated_conversions` is null or `generated_conversions` = or JSON_TYPE(generated_conversions) = 'NULL')

That arrow syntax is unfamiliar to me. Not sure if it's valid in MySQL but not MariaDB or if something got merged in #2384 funny, but it isn't working with the above version of MariaDB.

mperezsc commented 3 years ago

I think I am experiencing a related issue. Just upgraded to v9. I was able to run the migration as described in upgrade instructions and proceeded with the next needed steps for upgrading without problem. Checked that the new column appears in media table without issue. No problem there. The problem is that all my test are failing now, and the error is alike what you are seeing. Context: Laravel 8.40 PHP 8.0.6 Using Laragon with mysql 5.7.24 I use "RefreshDatabase" in my test class

Error output Illuminate\Database\QueryException: SQLSTATE[HY000]: General error: 1 near ">": syntax error (SQL: update "media" set "generated_conversions" = custom_properties->"$.generated_conversions", "updated_at" = 2021-05-10 16:22:09 where "generated_conversions" is null or "generated_conversions" = or JSON_TYPE(generated_conversions) = 'NULL')

I am quite a noob with this. Is it possible that the migration is "compatible" with mysql but not with the memory database phpunit uses? I'll show an extract of phpunit.xml: `

<server name="BCRYPT_ROUNDS" value="4"/>
<server name="CACHE_DRIVER" value="array"/>
<server name="DB_CONNECTION" value="sqlite"/>
<server name="DB_DATABASE" value=":memory:"/>
<server name="MAIL_DRIVER" value="array"/>
<server name="QUEUE_CONNECTION" value="sync"/>
<server name="SESSION_DRIVER" value="array"/>
<env name="DB_CONNECTION" value="sqlite"/>

`

Any info on how to proceed? Thanks!

freekmurze commented 3 years ago

This package requires MySQL

ethanclevenger91 commented 3 years ago

For anyone here who isn't familiar with the syntax and wants to know exactly what the issue is -

The -> operator in MySQL is the JSON_EXTRACT operator. Syntactic sugar currently not supported in MariaDB. See this documentation for understanding how it works in MySQL and this feature request for its status in MariaDB.

Seems to me that dropping it for JSON_EXTRACT in full would be a trivial change to this package to support MariaDB, but MariaDB has dropped any notion of being a MySQL drop-in replacement, so incompatibilities are only likely to become more numerous.

ethanclevenger91 commented 3 years ago

@mperezsc this syntax was introduced in MySQL 5.7.9. Your PHPUnit configuration is using SQLite for the database, not MySQL.

mperezsc commented 3 years ago

@ethanclevenger91 Thanks for the explanation. As a programmer hobbyist is hard to be aware of all these details.

haleyngonadi commented 3 years ago

@ethanclevenger91 Thanks for taking your time to explain. What solution did you use in the project?

ethanclevenger91 commented 3 years ago

@haleyngonadi I'll probably be migrating to MySQL. I don't know if @freekmurze would entertain a PR to add MariaDB compatibility. I'm pretty sure it has the same features, just not the pretty syntax, so the functionality would likely be maintained.

haleyngonadi commented 3 years ago

@haleyngonadi I'll probably be migrating to MySQL. I don't know if @freekmurze would entertain a PR to add MariaDB compatibility. I'm pretty sure it has the same features, just not the pretty syntax, so the functionality would likely be maintained.

Thanks for your reply. I migrated and now it works fine :)

pepijn-vanvlaanderen commented 3 years ago

A small rewrite will fix this for MariaDB: 'generated_conversions' => DB::raw("JSON_EXTRACT(custom_properties, '$.generated_conversions')"),

ethanclevenger91 commented 3 years ago

@pepijn-vanvlaanderen That would get the migration functional, yep, but @mperezsc's comment implies this exists in the plugin code itself somewhere, too. I haven't had a chance to look for it.