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

Add support for modifying the identifier delimiter #1061

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

orware
Copy link

@orware orware commented Sep 12, 2022

You can test the changes made in this PR directly, but the actual test suite may be expecting the SQL-92 specific syntax so it may not be able to directly run the tests when the identifier delimiter has been changed since the generated queries likely won't match anymore.

But by default, the changes proposed here should allow the existing test suite to continue to pass without any changes, since it should still be using the double quote identifier delimiter.

Taking input from Medoo#1044, this adds support for users to modify the quote identifier used for their quotes with databases that might not support the assumed SQL-92 standard double quote delimiter for identifiers.

While I think switching databases into the SQL-92 mode is a good approach generally for making maintenance on the Medoo side simpler, so that the code can be generalized more easily, it's not a workable approach in all situations as described in the issue above by the two PlanetScale users that have run into difficulties.

Similarly though, I could see a Microsoft SQL Server user possibly not wanting to do the same and it causing an issue somehow, although this particular situation does appear to be a bit more PlanetScale specific since the ability for the ANSI_QUOTES option is not supported at this time, nor does it seem to be something that will readily become available in the near future. We've documented this limitation on our MySQL Compatibility page.

Many libraries do allow for the customization of this identifier for their database-specific drivers, but I do understand the Medoo project's desire to keep things as simple and self-contained as possible and I hope the code suggested in this Pull Request is simple enough that it may be considered for users.

// Require Composer's autoloader.
require 'vendor/autoload.php';

// Using Medoo namespace.
use Medoo\Medoo;

// Connect the database.
$database = new Medoo([
    'type' => 'mysql',
    'host' => 'localhost',
    'database' => 'name',
    'username' => 'your_username',
    'password' => 'your_password'
]);

// Usage example of new setIdentifierDelimiter() method:

// For PlanetScale:
$database ->setIdentifierDelimiter('`');

// For something like SQL Server:
$database ->setIdentifierDelimiter('[]');

// You can also use it in a chained manner if you really wanted to (the delimiter would continue to use the new setting afterward):
$database ->setIdentifierDelimiter('`')->select("my_table", '*');

// The changes also correctly adjust the quotes when using the table.column notation in your column list:
$database ->setIdentifierDelimiter('`')->select("my_table", 'my_table.id');

@orware
Copy link
Author

orware commented Sep 12, 2022

Added a small correction in columnQuote() just now.

Didn't realize it until I was eating lunch a few moments ago that the logic was wrong for the potential SQL Server scenario:

// Example:
$data = $db->setIdentifierDelimiter('[]')->select("my_table", 'my_table.id');

// With debugging before the fix:
SELECT [my_table[.]id] FROM [my_table]

// After:
SELECT [my_table].[id] FROM [my_table]

@chriscct7
Copy link

This works great! @catfan can this be put into a release?

@catfan
Copy link
Owner

catfan commented Sep 15, 2022

@orware Thanks for this PR.
@chriscct7

However, it's no need to use a method to set the quotation identifier, because it will only call once in most cases. It can be better to set it from the connection option.

I will design a better way to achieve this, and test more other databases for this feature.

Of course, @chriscct7 can use this PR just now for your migration work.

@catfan catfan added the feature label Sep 18, 2022
@chriscct7
Copy link

@catfan is there a chance we can get this or something compatible with this like the approach you mentioned merged?

@catfan
Copy link
Owner

catfan commented Dec 7, 2022

@chriscct7 You can switch to @orware branch https://github.com/orware/Medoo.git and use it directly.

@chriscct7
Copy link

Hi there,
I've been using the orware branch for a while, but because it doesn't track in the other changes to the Medoo library, I'd like to circle back on seeing if this could be merged in or if the alternative method per connection you had mentioned has been implemented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants