Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
The philosophy in the core database layer is to encapsulate all field names with brackets, then allow the database to convert those into quotes if needed.
There are core queries that contain sql server reserved words. Either these need to be quoted, or the database driver needs to maintain a list of all reserved words, search every query, and encapsulate any matches.
One example is:
SELECT data FROM {config} WHERE name = :name found in core/tests/Drupal/KernelTests/Core/Config/Storage/DatabaseStorageTest.php. The word ‘data’ is a MSSQL reserved word.
Child issues
- #3152415: Bracket-encapsulated field names for static queries in core/lib/Drupal/Core
- #3152398: Change static queries to dynamic queries in core/tests/Drupal
- #3152390: Bracket-encapsulated field names for static queries in core/tests/Drupal/KernelTests/Core/Database
- #3160267: Change static queries to dynamic queries in core/modules/{every module}/tests
- #3160271: Bracket-encapsulated field names for static queries in core/modules/{every module}
- #3189880: Use square brackets syntax in sql queries
| Comment | File | Size | Author |
|---|
Comments
Comment #2
beakerboyComment #3
daffie commentedAnd Drupal loves to to call fields "data".
In #2986452: Database reserved keywords need to be quoted as per the ANSI standard we removed the whole "maintain a list of all reserved words" stuff and fields must be encapsulated with square brackets. Changing the queries from Connection->query() to Connection->select() is not allowed. The problem is that queries run with Connection->select() are a little bit slower then the ones run with Connection->query(). If we are going to do this then all the Connection->query() queries must change to using the quare brackets. This will be a very big change and a 2MB patch is not reviewable! Therefor my suggestion would be to change this isssue to a meta issue and start thinking how we can best divide it up into smaller sub-issues. Best ask one of the core committers for advice for the best way to divide it into sub-issues.
Comment #4
beakerboyThis patch covers the cases of ‘data’ that are preventing SQL Server from running tests.
Comment #5
beakerboyComment #6
beakerboyComment #7
daffie commented@beakerboy: There are a lot more queries in core with fields that need to be put in between square brackets. What is your plan for that? The change is the current patch look good.
Comment #8
beakerboyI posed that question to @alexpott. He suggested moving anything that is “not in the critical path” to dynamic (which I know you’d be happy to hear). Don’t know what the “plan” is though. The topic is on slack right next to the one regarding this issue. I need this to even begin testing sqlsrv on D9.
Comment #9
daffie commentedTalked to @alexpott on Slack and we change every static query to dynamic. He shall then review the patch and say which ones are in the critical path. Does we need to change back.
My suggestion would be to change this issue to a "META" issue and create sub-issue's with the actual patches in them. The question who are we going to split it up into smaller pieces. Some of the static queries need to be converted to an entity query. Like \Drupal\node\Plugin\Action\AssignOwnerNode::buildConfigurationForm().
My suggestion for splitting up the sub-issues:
- core/includes
- core/lib/Drupal/Core (without core/lib/Drupal/Core/Database)
- one for the module's: action up to and including comment
- one for the module: dblog
- one for the module's: forum up to and including locale
- one for the module's: migrate up to and including migrate_drupal
- one for the module: node
- one for the module's: path up to and including system
- one for the module's: taxonomy up to and including user
- one for the module: views
- core/tests/Drupal/FunctionalTests
- core/tests/Drupal/KernelTests (without core/tests/Drupal/KernelTests/Database)
@Beakerboy What do you think of my plan. Improvements are welcome.
Comment #10
beakerboyMy plan was to make this a “single issue” to get my immediate roadblock fixed. I made a separate “plan” issue (linked as related to this one) #3151225: Quote identifiers in core static queries..
Other than that, I like it!
Comment #11
daffie commentedComment #12
daffie commented@alexpott said on Slack:
and
What do you prefer @Beakerboy: writing the patch or reviewing the patch?
Comment #13
beakerboy@daffie, I do not have the time or resources to write a patch of this scale, but can push things along. I basically use GitHub as my API and do most of my coding on my iPhone...sometimes using ssh/vim into a hobby server to create a patch, which I use safari to upload to Drupal. My focus is to ensure the sqlsrv driver continues to work, because I use it for my job. I can’t dive into major infrastructure refactoring. I will certainly test what I can with the tools I have and provide feedback.
Comment #14
daffie commentedChanging this issue to a META issue.
Created 2 child issues: #3152398: Change static queries to dynamic queries in core/tests/Drupal and #3152390: Bracket-encapsulated field names for static queries in core/tests/Drupal/KernelTests/Core/Database.
Comment #15
daffie commentedComment #16
daffie commentedComment #17
daffie commentedAdded 2 new child issues: #3160271: Bracket-encapsulated field names for static queries in core/modules/{every module} and #3160267: Change static queries to dynamic queries in core/modules/{every module}/tests.
Comment #19
daffie commentedAdded #3189880: Use square brackets syntax in sql queries.
Comment #20
beakerboyAll the child issues are marked as closed. So I believe this is done.
Comment #21
daffie commentedWe have only #3130579: Make Drupal\Core\Database\Schema work with reserved keywords for naming and then core works with reserved keywords for naming.