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

Resolve mariadb for mysql contextually #164

Merged
merged 11 commits into from
Sep 20, 2023
Merged

Conversation

apotek
Copy link
Contributor

@apotek apotek commented Sep 15, 2023

Description

Adds a Robo task that provides a facility for being able to choose which binary to use depending on environment.

Motivation / Context

I wanted to get ahead of the fact that tugboat will soon be switching to mariadb binaries with no symlinking from mysql.

Which made me think it would be good to create a Robo task/utility for being able to switch between binaries, depending on the operating environment.

I used this also an a chance to get to know Robo just a bit. I could obviously have implemented this in a simpler way just for our use case.

I was going to contribute the "Alternatives" task as a Robo task to packagist and then import it, but I felt at this early stage it might be better to keep things simple and just add it to Usher.

Sounds good, but why are there so many changes to databaseRefreshTugboat?

I noticed that there was no consistent handling of "result" in that function. Result kept getting overwritten, meaning that the returned result at the end was basically meaningless. On top of that, if an exception occurred in attempting to get the dbPath, and there was only one site, there would be a return of NULL, which broke the function signature.

Lando?

I also added a .lando file so I could easily test some robo stuff in situ without having to import usher into another project just to test something.

Resolves: #159

And use this task to allow us to context-sensitively
switch between mysql and mariadb depending on what is
available on the image.
@apotek apotek changed the title Resolve mariadb for mysql Resolve mariadb for mysql contextually Sep 15, 2023
@ghost
Copy link

ghost commented Sep 15, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@apotek apotek self-assigned this Sep 15, 2023
@apotek apotek added the enhancement New feature or request label Sep 15, 2023
@apotek
Copy link
Contributor Author

apotek commented Sep 16, 2023

phpstan is throwing an error that I disagree with:

phpstan:

Error: Variable $dbPath in isset() always exists and is not nullable.


Line src/Robo/Plugin/Commands/DevelopmentModeBaseCommands.php


140 Variable $dbPath in isset() always exists and is not nullable.


But $dbPath will certainly not exist, if an exception occurs on line 133. So we need the isset() on line 140.

            try {
                $dbPath = $this->databaseDownload($siteName);
            } catch (TaskException $te) {
                $this->yell("$siteName: No database configured. Download/import skipped.");
                $result_data->append($te->getMessage());
                // @todo: Should we run a site-install by default?
                continue;
            }
            if (!isset($dbPath) || !is_string($dbPath) || strlen($dbPath) == 0) {

… nullable

phpstan complains about line 140 in DevelopmentModeBaseCommands.
I could not find any specific rule indicator to silence the
complaint. So I had to ignore the whole line.
phpstan isn't good at ascertaining state of variable definition
in try{}catch{} scenarios
Copy link
Contributor

@adamzimmermann adamzimmermann left a comment

Choose a reason for hiding this comment

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

Some questions (mostly out of my own ignorance), but this is great work. 👏🏼

Perhaps we can begin using this as is, but also begin working on trying to get something like this into Robo as a Plan B/long-term play?

.lando.yml Show resolved Hide resolved
src/Robo/Task/Discovery/Alternatives.php Outdated Show resolved Hide resolved
src/Robo/Task/Discovery/Tasks.php Show resolved Hide resolved
Co-authored-by: Adam Zimmermann <adam@chromatichq.com>
src/Robo/Plugin/Commands/DevelopmentModeBaseCommands.php Outdated Show resolved Hide resolved
src/Robo/Plugin/Commands/DevelopmentModeBaseCommands.php Outdated Show resolved Hide resolved
src/Robo/Plugin/Commands/DevelopmentModeBaseCommands.php Outdated Show resolved Hide resolved
src/Robo/Task/Discovery/Tasks.php Show resolved Hide resolved
.lando.yml Show resolved Hide resolved
src/Robo/Plugin/Commands/DevelopmentModeBaseCommands.php Outdated Show resolved Hide resolved
@apotek apotek requested a review from markdorison September 18, 2023 22:00
@apotek
Copy link
Contributor Author

apotek commented Sep 19, 2023

Some questions (mostly out of my own ignorance), but this is great work. 👏🏼

Perhaps we can begin using this as is, but also begin working on trying to get something like this into Robo as a Plan B/long-term play?

The "official" way to add tasks, is to simply put what I have in src/Robo/Task/Discovery into a php package with a robo-task type. So that would be an open source contrib and would then not need to be approved by Robo maintainers. But I suppose I could fork and make a merge request too. I'm open to it, if only because I think it would be interesting to see all the feedback about what I have done wrong (which would teach me more about Robo).

@adamzimmermann
Copy link
Contributor

I'm going to defer final approval to @markdorison here. Looking good to me though.

Copy link
Contributor Author

@apotek apotek left a comment

Choose a reason for hiding this comment

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

@markdorison I left a question re Symfony versus php-fpm container style.

.ddev/config.yaml Show resolved Hide resolved
@apotek apotek requested a review from markdorison September 20, 2023 03:14
@markdorison markdorison merged commit 8c6d485 into 3.x Sep 20, 2023
4 checks passed
@markdorison markdorison deleted the resolve-mariadb-for-mysql branch September 20, 2023 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make databaseRefreshTugboat() able to handle non-mysql database client names
3 participants