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

feat: support defining MySQLi flags #11383

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

wilr
Copy link
Member

@wilr wilr commented Sep 14, 2024

Description

Adds the ability to define mysqli flags (and socket) see #9394

Manual testing steps

To test this locally, you can set this via config.yml and verify in the MySQL connector that the flag is present

SilverStripe\ORM\Connect\MySQLiConnector:
  flags:
    - MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT

Issues

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

Comment on lines 84 to 85
$socket = static::config()->get('socket');
$flags = static::config()->get('flags');
Copy link
Member

@GuySartorelli GuySartorelli Sep 18, 2024

Choose a reason for hiding this comment

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

It seems weird to use config for this when everything else about a database connection is declared using env vars. Can you please explain your thinking here?

Copy link
Member Author

Choose a reason for hiding this comment

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

All the other db specific values ssl_cipher_default, connection_charset are config values so lets keep consistent

Copy link
Member

Choose a reason for hiding this comment

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

I feel like the socket you're using to connect has more to do with the environment-specific infrastructure, while the charset your connection uses is more to do with how the data is stored.

i.e. you want the data to be consistent regardless of what you're connecting to - but each environment may want to connect to a database on a different port.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah either or. Though you can always set config based on env vars so config gives a bit more flex.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer for everything to do with what database you're connecting to to be consistent. For CMS 5 that means explicitly using ENV vars.

We can change it in CMS 6 to use config which defaults to those env vars, if you want config.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure so SS_DATABASE_SOCKET How do you imagine the SS_DATABASE_FLAGS to work? I don't think it would function as a string.

Copy link
Member

Choose a reason for hiding this comment

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

We could either introduce some functionality to allow array values from env vars, or more simply, just use a comma (or other character) delimited string. Split by the delimiter, use constant() like you're doing below to get the specific flags based on the names.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to env vars

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

LGTM and works as expected. Thanks!

Could you please also open a PR to update the changelog and add the new env vars to the env vars list?

Comment on lines +88 to +91
$flags = $flags ? array_reduce(explode(',', $flags), function ($carry, $item) {
$item = trim($item);
return $carry | constant($item);
}, 0) : $flags;
Copy link
Member

Choose a reason for hiding this comment

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

This isn't super easy to read but I won't ask for you to change it now. Just something to think about for next time.

@GuySartorelli GuySartorelli merged commit c7ba8d1 into silverstripe:5 Sep 18, 2024
14 checks passed
@wilr wilr deleted the features/9394-mysqli-flags branch September 23, 2024 20:55
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