🇬🇧United Kingdom @pstewart

pstewart → created an issue.

Hi all, I've been away from Drupal for a while, but will get this merged and released as soon as I can.

This looks to be two separate issues, I think escaped tables support should be addressed in a separate issue and MR. The SQL Azure version check looks good to go, but I'm not sure about strpos($table_string, '['), my instinct is that the $identifierQuotes variable should probably be involved.

Hi all, I'm one of the maintainers of the the sqlsrv module https://www.drupal.org/project/sqlsrv → and from what I can see this change shouldn't change anything with respect to that module for sites using SQL Server either as the primary database or as an auxilliary database. I suspect most users of the module are hosting in a Linux env and just using SQL Server for database (which is my own situation), however I'm guessing we're more likely to have above average Windows + ISS users versus the rest of the community, so I've posted in the #sql-server Slack channel to publicise this RFC.

'String or binary data would be truncated.' errors happen when an insert or update query is trying to set more data for a column than the size of that column allows. This shouldn't really ever happen for structured data (entities, field values etc), so if you're seeing it on entity tables then it's probably a bug on whatever module is providing the entity or field that is causing the error.

There is a possibility that you could run into this on cache tables, for example if a piece of rendered content is too large for data column in the cache_render table. It would have to be pretty big though, the data columns in cache tables use the 'blob:big' field type, which maps on to the varbinary(max) SQL Server type and can hold 2^31 - 1 bytes (i.e. 2GB). In this case clearing caches could help but if there's a particular piece of rogue content that's triggering it then that piece of content will need to be fixed.

I recommend you check the Drupal logs to work out the exact queries that are erroring so you can diagnose further, and checking how much storage your tables are using (i.e. in SSMS right click a table -> Properties -> Storage).

No, it's only a problem if you're using contrib modules that use REGEXP in their SQL queries. This status error is going to be downgraded to a warning - see 🐛 MSSQL Server Custom Functions Error on Status Report page should be a warning Active .

I think a new 5.0.x branch targetting ^10 || ^11 compatibility and dropping 9.x support is the way to go.

This primarily relates to the lack of a REGEXP operator in SQL Server. IIRC Drupal Core supports the REGEXP operator and technically requires it, however does not use REGEXP itself. However, some contrib modules might make use of it, so some sites may experience problems with any such modules if they don't have a CLR installed to provide a REGEXP function that the module can use. I think downgrading it to a warning instead of an error would be reasonable.

🐛 Upgrading to Drupal 9.3.x changes users table and locks users out of the site Needs work now fixed and released in 4.4.0.

I'm planning to mark all the issues resolved in 4.4.x as fixed when we do the stable release, which will hopefully be the same as rc1 if no problems arise.

Noting that the underlying issue that caused this problem in 🐛 Upgrading to Drupal 9.3.x changes users table and locks users out of the site Needs work has now been fixed in 4.4.x, however an update hook would still need to be implemented to facilitate upgrades to 9.3.x. By this point that's probably not going to happen as most affected people will already have done a manual workaround with 4.3, and D9 is now EOL. I'll leave this issue open for now for discoverability and in case anyone wants to have a go at implementing the required hook, otherwise this should be considered as 'Closed (won't fix)'. Anyone still running into this problem can either apply the workaround in #14 or attempt to run the pertinent lines from the original user_update_9301 implementation:

$connection = \Drupal::database();
$connection->schema()->dropPrimaryKey('users');
$connection->schema()->changeField('users', 'uid', 'uid', ['type' => 'serial', 'not null' => TRUE], ['primary key' => ['uid']]);

Hah, I should've checked the issue list before releasing rc1 shouldn't I lol. As this issue just relates to the test suite this doesn't affect anything there though, so rc1 is still fine. You're right that \Drupal::moduleHandler() and \Drupal::service('module_handler') are doing exactly the same thing - they both get the module_handler from the container, so we only need one of them.

Noting that MR87 is now committed on 🐛 Upgrading to Drupal 9.3.x changes users table and locks users out of the site Needs work , so this issue will be considered as resolved in the rc1 release.

pstewart → changed the visibility of the branch 3263493-upgrading-to-drupal to hidden.

Noting that most of the discussion has been happening on 🐛 SQL Server Identity error when updating to Drupal 10.1.0 RTBC , which includes successful testing of MR87. The merge request will be completed on this issue and once this issue has been closed as fixed the other issue will be closed as duplicate.

@glassb Thank you for doing this testing, it confirms we're getting exactly the behaviour we're aiming for (competing processes blocked during the update but then able to complete there inserts on the recreated table as soon as the transaction is done).

@solideogloria Thanks for the code review, I've now done the corresponding fixes and run phpcs on the file and picked up a few others too (the only two coding standards violations left are on comments for getPrefixInfo which I introduced on an unrelated issue, so I'll do a separate commit to fix those after MR87 is merged).

Given that the MR is associated with 🐛 Upgrading to Drupal 9.3.x changes users table and locks users out of the site Needs work I think it does make sense to commit the merge there and mark this issue as duplicate when we close it. I'll make sure everyone who has contributed on this issue is credited on 3263493 when we commit.

The lint fixes I've committed are trivial with respect to code changes (purely spacing and comment changes), so I'm happy to merge and move forwards with making a 4.4.0-rc1 release without any further testing. I don't have time to do that today so will pick that up in the week).

After I rebased to 4.4.x I adopted the following testing regime:

1. Composer install fresh instance of D10.0.x and complete site installation process to a new database. Confirm 10.0.x version of schema in use (i.e. dbo.watchdog.wid has type int).
2. Update composer.json to target 10.1.x, perform composer update
3. Perform drush updatedb, ensure all updates complete successfully.
4. Verify 10.1.x version of schema is now in use (i.e. dbo.watchdog.wid now has type bigint)

I did my initial test of MR87 after the 4.4.x rebase last week and it worked without issue. I've made a small change to rename the originial table at the start of the database transaction to ensure we get the schema lock on the table before doing the data copy, which I think should be enough to prevent any problems from other site activity making changes on the table in parallel with the update. I've tested that change this morning and it's worked fine: it's behaving exactly as expected (although worth noting that in my testing regime I don't have any other traffic going on when I perform drush updatedb)

In more general terms I'm happy that the existing work the MR does to recreate the table, copy the data, and recreate the PK and indices is working as expected in this scenario - the code looks good! The attached file shows the before and after of the watchdog table, and we can see that the wid column is recreated as the last column in the table with the bigint type, and the key and indices all match up. Not shown is the data, but I checked that too - the recreated rows in the table
all look OK.

If someone with a more real-world setup can give it a try that would be great - if we can get at least one RTBC vote I'm happy to commit this and make a 4.4.x-rc1 release.

@.Kris. What version of SQL Server are you currently using? Note that JSON support is only available in SQL Server 2016 (13.x) and later.

I've rebased MR79 to 4.4.x, and opened MR87. I'm not really sure how to link that up to the drupal.org issues, but will be working on it in the corresponding rebased issue branch.

Thinking out loud here: what happens to a query blocked by a table lock when we eventually drop the locked table in favour of the replacement table?

(Also apologies for being AWOL - I've had to pivot to other projects and have had very little time to spend on Drupal for the last few months, I'll hopefully be able get some more Drupal time in before the end of the year as I still need to finish my D10 upgrade!)

Hi @wilco, thanks for working on this. OFFSET/FETCH has been support by SQL Server since 11.x / 2012, so only SQL Server 2008 and older versions would need this workaround. Given that such old versions are long since out of support and don't receive security updates from Microsoft, I would strongly recommend migrating to a newer SQL Server version. I think the minimum version targeted by previous maintainers is SQL Server 2016, and that's a hard minimum for Drupal 10 / 4.4.x when using SQLServer for the Drupal database due to the JSON support requirement.

As such I'm not inclined to bring this workaround back in to current versions of the module, however the patch does look OK otherwise so should serve you well in the interim while you figure out switching to a newer SQL Server version, and anyone else in a similar situation.

Noting that in my haste I made the try / catch here catch the exception but didn't include a variable for the exception object. This works OK in newer PHP versions (e.g. 8.0+), but causes a parse error on older php versions where the exception variable is required (e.g. 7.4 and below). As current Drupal 9 versions only officially support 8.0+ and 7.4 is EOL this is unlikely to be causing many people problems, but if anyone is running into this then my recommendation is to downgrade sqlsrv back to the previous version as php 7.4 shouldn't be affected by the ATRR_STRINGIFY_FETCHES issue in any case. I'll open a follow-up for fixing the php 7.4 parse error.

Tagged and released 4.3.4 and 4.4.0-beta2

Committed try / catch approach. Will now make a new 4.3 release and merge to 4.4 branch.

Update: it turns out this was picked up in the Laravel community last week. The change introduced in php 8.1.22 / 8.2.9 was that PDO has now started passing though the stringify attribute to the driver's set attribute method, which has exposed a bug in the Microsoft PHP pdo_sqlsrv driver whereby this attribute isn't being handled by it's set attribute method implementation. More discussion can be found on laravel issue #47937.

There is a pull request to fix the issue on the pdo_sqlsrv driver, which is awaiting review: Fixed error when using PDO::ATTR_STRINGIFY_FETCHES #1468

Ultimately this whole thing should resolve when the PR is accepted and Microsoft release a new version of the pdo_sqlsrv ext with
the fix, however that could take a while so we'll workaround in the meantime.

It's unclear to me what PDO::ATTR_STRINGIFY_FETCHES defaults to, so I'd rather not risk unintended side effects by removing it altogether. The exception occurs after the database handle's internal stringify attribute is set, so my preference is to continue to set the attribute from our code but put a try/catch around it to disregard the exception.

It looks like there was a bug fix in php 8.1.22 relating to this pdo attribute that might be responsible - I've not dug into it yet but have added a note on the relevant issue: After php8.1, when PDO::ATTR_EMULATE_PREPARES is true and PDO::ATTR_STRINGIFY_FETCHES is true, decimal zeros are no longer filled #11587.

For anyone affected by this and concerned about any potential side-effects of not setting the attribute, downgrading back to php 8.1.21 is an alternative option to try.

I've confirmed that the issue for me was triggered this morning by OS package updates, which involved php 8.1 (8.1.21 to 8.1.22) and msodbcsql17 (17.10.2 to 17.10.4). I was able to bring my site back online by commenting out the setAttribute call on line 208 of src/Driver/Database/sqlsrv/Connection.php as per @jurgenhaas 's issue fork. For the benefit of anyone unfamiliar with patching looking to bring their site back online in a hurry:

  /**
   * {@inheritdoc}
   */
  public function __construct(\PDO $connection, array $connection_options) {
    // COMMENT OUT THIS LINE --> $connection->setAttribute(\PDO::ATTR_STRINGIFY_FETCHES, TRUE);
    parent::__construct($connection, $connection_options);

    // This driver defaults to transaction support, except if explicitly passed
    // FALSE.
    $this->transactionalDDLSupport = !isset($connection_options['transactions']) || $connection_options['transactions'] !== FALSE;

    // Store connection options for future reference.
    $this->connectionOptions = $connection_options;
  }

It's possible there might be some very specific side-effects form disabling this attribute on some site (most likely around truthiness of numeric 0 vs stringified '0'), however I suspect the impact of any such side-effects is likely to be low (particularly when compared to a broken website).

I'm seeing the same error message on my production site, with errors starting at 6:22am. Am currently investigating whether an automated os packages upgrade has triggered.

Thanks for reporting this Jerry. I think you're right to link this to #3263493 🐛 Upgrading to Drupal 9.3.x changes users table and locks users out of the site Needs work ; in that issue the problem arises from a change to the users table where the uid column was changed from a plain integer column to an autoincrementing serial column, while in this one the change is alteration of the type of an existing column, but in both cases the root cause is the same: changeField conducts the change by creating a new field with the new spec, copying the data over from the old field to the new field, and then dropping the old field. Whether we're coming from an existing identity field or a non-identity field we get the same problem: to create a new identity field we need to recreate the table.

In the watchdog case where we're changing the type of the field another option is possible: with SQL Server 2016 or newer:

ALTER TABLE watchdog ALTER COLUMN wid bigint WITH(ONLINE=ON);

There are downsides to this though: it can potentially be quite slow, and doesn't solve the index problem either (so doesn't help in most use cases where the identity column is also the PK).

On the other hand, I can't see the table recreation method being any faster. There's also potential for data loss with a recreated table unless we can guarantee no inserts can happen on the original table between us copying the data over and renaming the old table to take over from the new one. There is a strategy to change a column from int to bigint without downtime where we use triggers to ensure we're catching changes to the original table while we're copying data over.

I will try and spend some time on this later this week and see what changes we need to make to the table recreation approach in MR79.

Duplicates #3298153 📌 Automated Drupal 10 compatibility fixes Fixed , fixed on 4.4.x. The 4.4.x branch is where D10 compatability is happening, so this is a Won't Fix for 4.3.x.

Fix in 4.3.x has been merged to 4.4.x and included in 4.4.0-beta1

OK, let's get this committed and tag a new release

I've been investigating this, and although I've not been able to recreate I can see how the problem can arises if the table name passed to
Schema::createFieldSql is prefixed with a schema, as the code uses the table name directly when creating the constraint name:

      if (isset($spec['default'])) {
        $default = $this->defaultValueExpression($sqlsrv_type, $spec['default']);
        $sql .= " CONSTRAINT {{$table}_{$name}_df} DEFAULT $default";
      }

What I'm unclear about is how $table is acquiring a schema prefix, as I don't see this happening in my dev environments either with or without the schema being defined in settings. The one thing I've not tried is adding a schema via the prefix setting.

@drp_distruptor are you seeing this problem on a pre-existing site or on a new install, and do you see the same problems with the previous release 4.3.1? Also please can you let me know what database settings you are using (without sensitive details of course).

@arne_hortell your patch from #15 appears to be the same as @ncac's original patch apart from some trailing newlines, did the revised patch from the merge request actual work for you in the end?

I have now managed to recreate this error on my D10 development environment by explicitly removing the schema key from the database settings in settings.php, then clearing all caches from the web interface. Drush then failed as described above. I then applied the diff from MR84 and confirmed that it resolved the problem.

@ncac if you're able to confirm the current diff from MR84 is working then that would be great, however I'm pretty satisfied this fix is the way to go so if I don't hear back I'll go ahead and commit this and make a new release.

This is effectively a duplicate, but I'll keep it open as it is tagged explicitly for the 4.4.x branch. When we commit the fix to 4.3.x this will also get committed on to the 4.4.x branch.

Related fixes to unit and kernel tests now committed.

pstewart → created an issue.

I should note that while I've succesfully used #10 in a development environment to solve the stalled orders issue, I've never put it into production for a few reasons:

• With the patch I then ran into race condition problems (see #3043180 🐛 The changes made to the order on the onNotify method are not applied on the onReturn method Fixed ). There has been progress on that issue recently, however I am running a bit behind on Commerce at the moment so haven't tested with the latest fixes.
• The patch does make a schema change, which I'm reluctant to put into to my production database while there is doubt over the approach taken.
• While the patch tries to take an approach consistent to how orders become placed on the main authorize + capture workflow, I agree with @zaporylie that a more workflow agnostic approach is required, perhaps something along the lines of what's been done so far or a simpler core-provisioned event subscriber approach, but either way has some configurability to allow authorization workflow site builders to opt in or out of order placement by IGN depending on what the offsite gateway supports.

I've pushed a change to the issue fork to simplify the fix, which seemsto be working ok in a D9 test environment. Please can those affected try the issue fork patch to test?

@arne_hortell can you confirm which Drupal and Drush versions you were running when you experienced the Drush errors?

A follow-on to this job is needed, as fieldExists should also be schema aware in case of tables in different schemas having the same name, which could lead to a similar false positive for a column existing on the target table.

I've taken a quick look at this in my D10 test environment and am not seeing the problem, but I'm guessing this maybe that with drush + D9.3 tableExists is getting hit before something else that calls into getDefaultSchema which causes the error, while with drush + D10 it seems that getDefaultSchema is getting hit before tableExists so the bug doesn't manifest (@ncac I'm guessing it's a similar reason why you're seeing the problem in drush but not on the site itself). The problem has almost certainly materialised due to the change in #3259155 🐛 Schema::tableExists doesn't check schema Fixed which now does check the schema explicitly in tableExists (although I realise now that the solution there is incomplete as we should be doing the same check in fieldExists, that's a separate problem).

Implementing getPrefixInfo is definitely the way to go here, however I think we can simplify a bit by just calling getDefaultSchema to ensure the defaultSchema variable is set, then delegating back to the parent.

I'll take a closer look next week when I have some more time to look at Drupal stuff.

@JerryACSA I think everything you have listed is committed in to the 4.4.x branch:

• Schema.php fix: Committed in efb2358 (this was a small enough change I didn't bother opening an issue for it)
• Call to a member function fetchField() on null: Patch committed in #3291199 🐛 Call to a member function fetchField() on null Fixed
• Table prefix issue: Patch committed in #3362458 🐛 Table prefix handling not compatible with removal of per-table prefixing Fixed . It looks like you've taken a similar approach, however I think your version might have issues if a database-wide prefix is in use.
• JSON Requirements problem: Patch committed in #3332118 📌 Database JSON support required Fixed
• Version requirement: Committed in dff92b0 (opens the 4.4.x branch)

It's encouraging to hear you've been running successfully for a while with essentially the same set of changes as the current 4.4.x branch. If you're able to try out the current 4.4.x-dev that would be great!

OK so it looks like rebasing a merge request to a different branch is a major faff so I'm just going to commit this directly to 4.4.x to facilitate a 4.4.x-dev release. Status remains as 'Needs Review' while the 4.4.x branch is in testing.

I'm going to commit this patch to 4.4.x to facilitate a dev release, marking this issue as Needs Review for testing.

pstewart → created an issue.

I've now committed this to 4.4.x for inclusion in the next dev release. I've encountered no issues with this when running the Drupal 10 installer with an SQL Server database, and have confirmed the Status Report shows 'Database support for JSON' as being 'Available'. I think this is therefore all done, however I'll keep this issue at 'Needs Review' while the 4.4.x branch is in development and testing, and we'll move to 'Fixed' once we're ready to release 4.4.0 as stable.

Committing #2 to 4.4.x for inclusion in the next 4.4.x-dev release, but will keep the issue as Needs Review while we're testing and for any further changes we might want to make on this.

pstewart → created an issue.

A quick update for everyone: I am now a maintainer on the project, and have just release 4.3.2 with existing RTBC bug fixes plus the feature for adding SQL Server specific DSN options. D10 compatibility work will progress in a new 4.4.x branch, with 4.3.x potentially receiving one more release to resolve the users table uid schema change issues.

Thank you @Beakerboy!

Having read through this issue, it looks like the following actions are needed to resolve:

1. Test and review MR 79 to ensure sqlsrv can cope with schema changes that convert columns to serial.
2. Implement an update function to ensure the user uid column is converted to serial as per user_update_9301. This update function should ensure it only attempts to do work if the column is not already an appropriate identity column in the database (i.e. if the site admin has already fixed the schema manually to work round the problem while awaiting the fix from this issue).

Is this correct?

Provision of these additional sqlsrv specific options is being addressed in #3291464 ✨ Support for SQL Server specific DSN connection options Fixed , so I think this is a duplicate?

@Beakerboy is correct in #4 🐛 Service not found exception Fixed , in that the intended approach for backend overridable services is that you would ask for the overridable service from the container (views.date_sql in this case) and you would get the correct backend version of the service corresponding to the preferred default backend configured in your site's services.yml, e.g.

parameters:
  default_backend: sqlsrv

This is presumably why the core backend implementations are marked private - they're only intended to ever be used via the backend override mechanism rather than injected directly at runtime. However this doesn't take into account use cases where the database driver is used to access a secondary database, where defining the default_backend in that way is not applicable (if the primary database is Mysql then the default backend still needs to be mysql).

Allowing the sqlsrv.views.date_sql to be public is therefore necessary to facilitate these kinds of use cases, so Maather's patch in #6 🐛 Service not found exception Fixed is the correct fix.

Ah, thanks for this. I couldn't find a way of discerning which maintainer has which status, so incorrectly inferred Damien's status as they show as the original project creator. The only maintainer I've not yet tried reaching out to is izaaksom, as they don't appear to be active (one issue credit on Drupal Core in 2016, no obvious activity in this project that I can see).

I have now also contacted Damien Tournoud, who I think is the project owner.

There is a #ldap channel on Drupal Slack, but pretty much nothing has happened on it since 2021. @grahl was previously an active developer and maintainer on the project, but does not appear to be a maintainer anymore, and I'm not sure which of the current maintainers are active (if any). This project is on my radar as my organisation's site depends on ldap and we're working on getting upgraded to D10, so I'll be taking a look at ldap D10 related issues soon and making what contributions I can.

I have previously contacted the most recently active co-mainter @Beakerboy prior to opening this issue through both their contact form and Drupal Slak, and have reached out to them again via their contact form now this issue has been opened. I have also reached out to @david_garcia via their contact form as the next most recently active co-maintainer.

pstewart → created an issue.

pstewart → created an issue.

Actually it may be possible to support 9.x and 10.x in the same branch by creating a wrapper interface using interface_exists.

Just noting this issue is a side-effect from 3185269 → , which removed the default value from the options array. THe patch copies over the relevant change that was made in the parent class, so is good to go.

It looks like a new branch will be necessary as we'll need to use the new Drupal\Core\Database\SupportsTemporaryTablesInterface interface which is not available in D9.5. See 3246056#6 🐛 deprecation notices in 4.3.x Needs work .

My suggestion is that we start a new 5.x series for D10 and beyond, with 4.3.x continuing for D9 support until D9.5 eol.

It looks like things have moved on a bit since Beakerboy opened this issue: queryTemporary is no longer deprecated per-se, however has been made optional in D10 by being moved to its own interface Drupal\Core\Database\SupportsTemporaryTablesInterface (see https://www.drupal.org/node/3316569 → ). As this interface doesn't exist in D9.5, it implies we're going to need a new branch as we can't just introduce it while still supporting D9.5.

Probs the way to go here is to amend the patch to drop the expectDeprecation for queryTemporary to resolve the issue for 4.3.x, and then address the support the new SupportsTemporaryTablesInterface in a new issue.

I've started a new issue for tracking work that needs doing on this project to get a Drupal 10 compatible release.

pstewart → created an issue.

I ran into some trouble applying #5 as the file was formatted as UTF-16. This version of the patch is reformatted as UTF-8, but is otherwise identical to #5. Can confirm it works as expected talking to an SQL Server 2017 database.

As far as I can tell the hasJson method is the only thing we need to do at this stage, other stuff relating to json appears to be still active in core at the moment, such as mapping json data types in the schema API etc.