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

Alternate MySQL 8 Patch (upstreaming from Silkscreen) #4932

Open
jlfranklin opened this issue Feb 3, 2021 · 4 comments
Open

Alternate MySQL 8 Patch (upstreaming from Silkscreen) #4932

jlfranklin opened this issue Feb 3, 2021 · 4 comments

Comments

@jlfranklin
Copy link
Member

Description of the need

In order to support SQLite in Silkscreen, this patch from d.o is needed to change the way database tables are prefixed. The introduction of the MySQL 8 patch added from #4238 conflicts with the prefixing patch breaking many of the tests.

Proposed solution

To continue supporting SQLite, the easiest thing to do is update Backdrop's MySQL 8 patch to match the MySQL 8 for Drupal 7 patch (commit), which does work with the prefixing patch above. In fact, both patches are already in D7.

This issue will include the MySQL 8 patch re-rolled and upstreamed for Backdrop matching what is slated for Silkscreen. The prefixing patch will not be included, as it is a non-trivial change to how prefixing is done, with no direct benefit to Backdrop.

Updating the MySQL 8 patch has the added benefit of not drifting too far from Drupal 7's database code, allowing Backdrop to enjoy additional upstream patches in the future.

@jlfranklin
Copy link
Member Author

jlfranklin commented Feb 3, 2021

The patch from #4238 and what D7 committed are not that far apart. However, just to make it easier to review and ensure the full D7 patch is correctly applied, the PR is split into four commits. The first two revert the existing MySQL 8 patch commits, the third applies the D7 patch, the fourth makes the minimum changes necessary to work with Backdrop.

Comparing the head of the PR branch with 1.x, the difference between the two patches can be seen. The primary difference is the D7 patch only quotes reserved words, not all table and field names.

@jlfranklin
Copy link
Member Author

(Side note: I have no objections to including the prefixing patch in Backdrop. However, I think it's out of scope for this issue, and should be considered separately.)

@indigoxela
Copy link
Member

To continue supporting SQLite, the easiest thing to do is update Backdrop's MySQL 8 patch to match the MySQL 8 for Drupal 7 patch

@jlfranklin do you see any chance to get the SQLite support working, without changing the Backdrop MySQL 8 support approach?

@jlfranklin
Copy link
Member Author

jlfranklin commented Feb 4, 2021

@jlfranklin do you see any chance to get the SQLite support working, without changing the Backdrop MySQL 8 support approach?

It would take some time to massage the prefixing patch in so it works correctly, and then every patch that comes along in the next two years runs the risk of needing similar work. There is already a follow-on D7 patch to fix some table quoting, not included in the current PR.

In particular, the changes to DatabaseSelectTestCase in the Backdrop patch are specific to MySQL. Silkscreen would need two versions of those tests, one for MySQL and one for everything else as the backtick quoting is "decidedly non-standard."

So, technically possible, but updating the MySQL 8 implementation has a far lower overall LoE. For Silkscreen, I think I'd have to choose between drifting from Backdrop or drifting from Drupal 7.

The good news is this PR changes the behavior of some calls compared to the original Backdrop patch, but doesn't remove or change the signatures of any APIs, and so is (mostly) backwards compatible.

I'm sorry, I should have run these tests when 1.18.0-preview was cut and addressed it before the actual release.

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

No branches or pull requests

4 participants