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

Handle MySQL commands without deprecation warnings #274

Closed
wants to merge 2 commits into from

Conversation

ton31337
Copy link

@ton31337 ton31337 commented Nov 8, 2024

With this fix:

% ./vendor/bin/wp db check --path=/tmp/wordpress
mariadb-check: ...

Issue: #271

@ton31337 ton31337 requested a review from a team as a code owner November 8, 2024 09:33
@ton31337 ton31337 force-pushed the feature/adopt_for_mariadb branch 8 times, most recently from 37e888b to f118585 Compare November 8, 2024 12:00
@ton31337
Copy link
Author

ton31337 commented Nov 8, 2024

Seems that lots of tests are failing due to MySQL connection.

@ton31337 ton31337 force-pushed the feature/adopt_for_mariadb branch 4 times, most recently from c2c919e to 2797400 Compare November 8, 2024 13:21
First look for a symlink and use it if it exists, otherwise use a given command.

With this fix:

```
% ./vendor/bin/wp db check --path=/tmp/wordpress
mariadb-check: ...
```

Issue: wp-cli#271

Signed-off-by: Donatas Abraitis <[email protected]>
Signed-off-by: Donatas Abraitis <[email protected]>
@ton31337 ton31337 force-pushed the feature/adopt_for_mariadb branch 2 times, most recently from 7a6d128 to ecfc4a9 Compare November 10, 2024 19:23
…mmand

Just directly call readlink() which returns the full path regardless if it's
a symlink or a regular binary.

Signed-off-by: Donatas Abraitis <[email protected]>
@mrsdizzie
Copy link
Member

I'm not sure it is right to silence the warning without addressing it -- the original issue is that right now if you install MariaDB it will create symlinks like:

stat /usr/bin/mysqldump
  File: /usr/bin/mysqldump -> mariadb-dump

and if you run mysqldump it gives a warning mysqldump Deprecated program name. It will be removed in a future release, use mariadb-dump instead

So this code gets rid of the warning temporarily by checking where the link points, but the actual problem the warning is telling us is that mysqldump link will be removed completely in a future release. This code still leaves wp db relying on the existence of a mysqldump link in order to work and it would still break when that link is removed. So silencing the warning seems worse, because it would then stop working without warning at some point in the future.

I think any change should actually address the warning and fix the code to know about MariaDB command names and if we should use those. I haven't looked close enough to see how we should decide that: If wp db should check and know what type of database is in use, or if it should be less clever and just check for the existence of mariadb-dump etc... in the case that mysqldump doesn't exist.

@ton31337
Copy link
Author

This code still leaves wp db relying on the existence of a mysqldump link in order to work and it would still break when that link is removed.

IMO, both methods should be combined to solve this issue. I mean following the symlink + handling the real mariadb* commands (which is annoying from MariaDB's perspective, but it is how it is).

On the other side, the current implementation is broken too if the symlink is removed completely by MariaDB.

If wp db should check and know what type of database is in use, or if it should be less clever and just check for the existence of mariadb-dump etc... in the case that mysqldump doesn't exist.

Yes, that's what I had in my head above, either be clever or dumb.

@mrsdizzie
Copy link
Member

Thank you for the PR / push to address this. This should be fixed by #275 which will select the proper command based on the current MySQL / MariaDB version

@mrsdizzie mrsdizzie closed this Nov 24, 2024
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