Add character set support for CSV with LOAD DATA in table imports - #20475

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.95%. Comparing base (9cb26dd) to head (3bf0b7e).

@@            Coverage Diff            @@
##             master   #20475   +/-   ##
=========================================
  Coverage     63.94%   63.95%           
- Complexity    16085    16089    +4     
=========================================
  Files           677      677           
  Lines         57830    57842   +12     
=========================================
+ Hits          36982    36994   +12     
  Misses        20848    20848           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

I'll look into the failing tests. It seems to mostly be focused on the test suite, namely the inclusion of non-typed data providers. I've found the issues, I'm pushing the new version ASAP.

We’ll also need to find a way to gracefully handle incorrect character set selections without triggering a large number of unfriendly database errors. One approach could be to display a clear, user-friendly error message when a character set issue occurs in the SQL query, such as: "The selected character set cannot represent all characters in the data!"

There are also some character sets that are unsupported or cannot be used with LOAD DATA. For example, UTF-16 is not supported by MySQL for LOAD DATA, although it is supported by MariaDB. In such cases, the current approach is to omit the CHARACTER SET clause and let the database handle the encoding, most likely defaulting to UTF-8.

This can become problematic when a user explicitly selects an encoding such as ISO-8859-7, since MySQL does not support it.

Update: The fix first maintains a whitelist of character sets supported by the selected database system, such as MariaDB or MySQL. If the user selects a character set that is not supported by their database system, a graceful error is displayed. However, if the selected character set is supported by the database but is incorrect for the actual file encoding, the application does not try to correct it and simply lets the user see the resulting errors from the SQL queries. This keeps the behavior consistent while clearly distinguishing unsupported database character sets from an incorrect user selection.

I was pretty sure I fixed all failing tests. What do these failures even mean? I ran the test suite locally, as one should and I can't pinpoint the issue! What does it mean the error was on line 0 and char 0?! I tested it on the web version of phpMyAdmin and obviously ran phpunit, phpstan, phpcbf on my PR and it was all fine. Psalm is complaining!

Error: src/Plugins/Import/ImportLdi.php:0:0: UnusedBaselineEntry: Baseline for issue "PossiblyUnusedReturnValue" has 1 extra entry. (see https://psalm.dev/316)
Error: src/Plugins/ImportPlugin.php:0:0: UnusedBaselineEntry: Baseline for issue "PossiblyUnusedReturnValue" has 1 extra entry. (see https://psalm.dev/316)

How are failures like this handled?

php -d xdebug.mode=off vendor/bin/psalm --set-baseline=psalm-baseline.xml --no-cache

This works, thanks!

php -d xdebug.mode=off vendor/bin/psalm --set-baseline=psalm-baseline.xml --no-cache

This works, thanks! However, what I am supposed to do if the CI is not ran from my machine?

Just run psalm without --set-baseline and it will show you the issues. There's also a composer command for the same.

php -d xdebug.mode=off vendor/bin/psalm --set-baseline=psalm-baseline.xml --no-cache

This works, thanks! However, what I am supposed to do if the CI is not ran from my machine?

Just run psalm without --set-baseline and it will show you the issues. There's also a composer command for the same.

Without --set-baseline, there seem to be no errors found. What do I do with this information?

That's right. I don't think Psalm reports unnecessary baseline entries. So best to just set the baseline once you are done.

That's right. I don't think Psalm reports unnecessary baseline entries. So best to just set the baseline once you are done.

Is it OK to commit a new baseline? I heard changes should be focused on their respective areas.

Yes, you even should. Baseline should always be kept up-to-date. If your code causes changes to the baseline then you should include it in the PR.

Yes, you even should. Baseline should always be kept up-to-date. If your code causes changes to the baseline then you should include it in the PR.

I'd like to thank you for helping me out. I'm a PHP developer and I've recently trying dipping my toes with open-source projects. I knew PHPStan and PHPUnit but certainly not Psalm and its baselines. I just wanted to fix an issue however I ended up wasting the time of others who had no business with me. I committed the brand new baseline and all signs are green.

I could have never imagined I would have received help and guidance in a pull request, but well here I am. I'd like to apologize to everybody whose time I wasted and also for the CI minutes I've wasted.

This PR is now complete and anybody is free to check it out. I've fixed the failing tests.

No problem. Regardless of the outcome of this PR, I am very happy that you would like to contribute to open source. Every contribution is very much appreciated. Don't worry about small mistakes; we all make them, especially at the start.