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 custom database drivers #6127

Open
wants to merge 4 commits into
base: 13.x
Choose a base branch
from

Conversation

bradjones1
Copy link
Contributor

The SQL dump command doesn't like if the driver is not one of the core set; this means you can't dump SQL when using a custom driver. Proposing a fix that will only be invoked if the driver is non-core.

@weitzman
Copy link
Member

weitzman commented Oct 7, 2024

Its been a long time, but i think I solved this via a composer.json autoload entry - https://gitlab.com/weitzman/drush-oracle-driver

@bradjones1
Copy link
Contributor Author

I think that's a bit of a different situation, TBH. This is a case where the underling db type is supported by Drush, it just doesn't know it can dump from it because the driver name is different from the "base" driver it extends. E.g. in this case it was sqlite - the custom driver needs to be named something else, but it's still sqlite under the hood.

@weitzman
Copy link
Member

weitzman commented Oct 7, 2024

Can't the custom driver create its own class that extends SqlSqlite and override the dumpCmd or dumpProgram() method?. This is what SqlMariaDb does - https://github.com/drush-ops/drush/blob/13.x/src/Sql/SqlMariaDB.php

@bradjones1
Copy link
Contributor Author

Except in many cases the custom driver is just providing some additional functionality and doesn't need to provide any custom logic for loading or dumping the db... it's just extending.

@weitzman
Copy link
Member

weitzman commented Oct 7, 2024

Your custom driver can do stuff before or after a call to parent::dumpCmd(), for example. It can be super thin like that mariadb driver is. Sorry, I'm not really following where custom drush db drivers are deficient here.

@bradjones1
Copy link
Contributor Author

No apology needed - these things can be hard to suss out. You make a good point that we should support custom drush DB driver logic - and should look for a class matching the pattern in the code to load up. But if your custom drupal DB driver is so thin that it needs not do any custom logic, there's no reason we should require developers to ship a boilerplate Drush db class (and, know they would need to do that) if they are extending a core db type.

@weitzman
Copy link
Member

weitzman commented Oct 7, 2024

I'm afraid we disagree. Extension via OO is a core principle we can and should rely on.

Also, there is no boilerplate in the custom class. It just contains the overridden method(s).

@bradjones1
Copy link
Contributor Author

Extension via OO is a core principle we can and should rely on.

I agree with this. But let's say I have a custom driver that exists only to extend sqlite with, say, an entity query condition.

Currently, if I try to drush sql-dump it errors, saying it can't find brads_sqlite driver. I would have to know to ship a Drush db driver class that is empty and only extends the base Sqlite Drush driver to be able to dump.

I'm suggesting that if there is no specific Drush driver found, and the driver extends a driver Drush ships with, use that.

Copy link
Member

@weitzman weitzman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, now I understand what you are proposing. Thanks for the patience.

src/Sql/SqlBase.php Outdated Show resolved Hide resolved
src/Sql/SqlBase.php Outdated Show resolved Hide resolved
@bradjones1
Copy link
Contributor Author

@weitzman I updated this per your feedback... I swallowed anticipated exceptions as suggested but I'm wondering if we shouldn't throw an exception instead of returning NULL here? I think we could provide a better DX in cases where there is no Drush driver found instead of the "can't X on NULL" that you get now if none is resolved.

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