Problem/Motivation

We should deprecate the functions in includes/schema.inc.

Just spinning off a follow up to #2124069: Convert schema.inc to the update.update_hook_registry service (UpdateHookRegistry)

Additional goal to simplify usage of "module schema" remains from drupal_write_record()

In comment #65 it was cleared that we need to split "update registry" and management ([un]instali, serving hook_schema()) functions

In comment #87 elaborated why drupal_get_module_schema() is not much popular in contrib and the helper class extracted to \Drupal\Core\Extension\ModuleSchema::getTables() static method

In comment #113 it was decided that the helper class should only be needed by tests and does not have to be a trait, so it was moved to \Drupal\TestTools\Extension\SchemaInspector::getTablesSpecification()

Proposed resolution

Deprecate the following functions from includes/schema.inc and create a new helper class with static method
\Drupal\TestTools\Extension\SchemaInspector::getTablesSpecification() to load install file and retrieve hook_schema() data for tests only.

  • drupal_get_module_schema() - No direct replacement is provided. Test code should use \Drupal\TestTools\Extension\SchemaInspector::getTablesSpecification() for introspection.
  • drupal_install_schema(), drupal_uninstall_schema(), _drupal_schema_initialize() - without replacements as should be implementation details of module installer

Remaining tasks

- polish comments
- add tests (for new class)
- commit

User interface changes

None

API changes

See proposed solution.

Data model changes

None

Release notes snippet

API to manage modules schema depreacted

CommentFileSizeAuthor
#132 2908886-132.patch19 KBandypost
#132 interdiff.txt569 bytesandypost
#131 2908886-131.patch19.04 KBandypost
#131 interdiff.txt3.97 KBandypost
#126 2908886-126.patch19.23 KBdaffie
#125 2908886-module-schema-125.patch19.25 KBandypost
#125 interdiff.txt2.93 KBandypost
#119 2908886-module-schema-119.patch19.1 KBandypost
#119 interdiff.txt8.05 KBandypost
#117 2908886-module-schema-117.patch19.1 KBandypost
#115 2908886-115.patch18.58 KBandypost
#115 interdiff.txt9.79 KBandypost
#111 2908886-111.patch18.55 KBandypost
#111 interdiff.txt599 bytesandypost
#108 2908886-108.patch18.55 KBandypost
#108 interdiff.txt1.29 KBandypost
#103 2908886-103.patch18.71 KBandypost
#103 interdiff.txt1.67 KBandypost
#100 2908886-100.patch19.05 KBandypost
#100 interdiff.txt1.84 KBandypost
#97 2908886-97.patch19.3 KBandypost
#97 interdiff.txt12.53 KBandypost
#92 2908886-92.patch18.12 KBandypost
#92 interdiff.txt652 bytesandypost
#90 2908886-90.patch18.11 KBandypost
#90 interdiff.txt1.14 KBandypost
#89 2908886-89.patch16.96 KBandypost
#89 interdiff.txt13.21 KBandypost
#88 2908886-88.patch19.05 KBandypost
#88 interdiff.txt4.1 KBandypost
#85 2908886-85.patch18.72 KBandypost
#85 interdiff.txt1.33 KBandypost
#83 2908886-83.patch18.88 KBandypost
#83 interdiff.txt13.95 KBandypost
#82 2908886-82.patch9.74 KBandypost
#82 interdiff.txt6.63 KBandypost
#81 encapsulation-2908886-81.patch7.2 KBmartin107
#81 interdiff-2908886-77-81.txt64.96 KBmartin107
#77 2908886-77.patch68.59 KBdaffie
#77 interdiff-2908886-75-77.txt9.99 KBdaffie
#75 2908886-75.patch68.48 KBandypost
#75 interdiff.txt2.1 KBandypost
#73 2908886-73.patch68.5 KBandypost
#73 interdiff.txt15.53 KBandypost
#71 interdiff-2908886-68-71.txt64.74 KBmartin107
#71 2908886-71.patch69.84 KBmartin107
#68 interdiff-2908886-67-68.txt24.21 KBmartin107
#68 2908886-68.patch71.84 KBmartin107
#67 2908886-67.patch67.19 KBmartin107
#61 2908886-61.patch67.03 KBandypost
#61 interdiff.txt603 bytesandypost
#60 2908886-60.patch67.03 KBandypost
#60 interdiff.txt9.53 KBandypost
#58 2908886-58.patch66.8 KBandypost
#58 interdiff.txt5.27 KBandypost
#56 2908886-56.patch66.07 KBandypost
#56 interdiff.txt760 bytesandypost
#55 2908886-55.patch66.12 KBandypost
#55 interdiff.txt650 bytesandypost
#54 2908886-54.patch66.15 KBandypost
#54 interdiff.txt1.39 KBandypost
#52 2908886-52.patch66.2 KBandypost
#52 interdiff.txt7 KBandypost
#49 2908886-49.patch60.1 KBandypost
#49 interdiff.txt1.07 KBandypost
#48 2908886-48.patch59.85 KBandypost
#48 interdiff.txt10.57 KBandypost
#46 interdiff.2908886.40-46.txt45.83 KBaleevas
#46 2908886-9.1-46.patch57.17 KBaleevas
#40 2908886-40.patch69.53 KBandypost
#40 interdiff.txt15.9 KBandypost
#39 2908886-39.patch68.34 KBvacho
#35 2908886-35.patch68.11 KBkim.pepper
#35 2908886-35-interdiff.txt1.67 KBkim.pepper
#33 2908886-33.patch67.83 KBkim.pepper
#33 2908886-33-interdiff.txt1.25 KBkim.pepper
#31 2908886-31.patch67.48 KBkim.pepper
#31 2908886-31-interdiff.txt13.98 KBkim.pepper
#30 2908886-30.patch66.04 KBkim.pepper
#30 2908886-30-interdiff.txt4.33 KBkim.pepper
#27 interdiff-2908886-23-27.txt4.81 KBmartin107
#27 2908886-27.patch63.19 KBmartin107
#24 2908886-23.patch62.81 KBandypost
#24 interdiff.txt7.01 KBandypost
#23 2908886-22.patch62.81 KBandypost
#23 interdiff.txt976 bytesandypost
#20 2908886-20.patch62.75 KBandypost
#20 interdiff.txt736 bytesandypost
#18 2908886-18.patch62.69 KBandypost
#18 interdiff.txt1.53 KBandypost
#15 2908886-15.patch62.49 KBandypost
#15 interdiff.txt19.32 KBandypost
#14 2908886-14.patch51.77 KBandypost
#14 interdiff.txt9.02 KBandypost
#12 2908886-12.patch45.47 KBandypost
#12 interdiff.txt17.64 KBandypost
#10 2908886-10.patch39.08 KBandypost
#7 2908886-7.patch39.1 KBmartin107
#7 interdiff-2908886-4-7.txt4.69 KBmartin107
#4 2908886-2.patch39.04 KBmartin107
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martin107 created an issue. See original summary.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

martin107’s picture

In relation to the parent issue

I am just splitting the change record into two so we can focus.

For the moment I have just cloned the change record and deleted reference to relevant function calls.

https://www.drupal.org/node/2970993

martin107’s picture

Status: Active » Needs review
FileSize
39.04 KB

Here is a first draft ... it will cause conflicts with the parent issue because both add parameters to the ModuleInstaller class

For review, here is a summary :-

A) I started with the big patch from the parent issue and then cut out the sections that were staying in the parent issue

B) I found a home for the functions listed in the issue summary. See the new service 'schema_installer' as defined by a new class \Drupal\Core\Extension\SchemaInstaller.

C) lots of 'plumbing in' and stitching the monster into a cohesive thing.

Still todo as it will cause problems and need a reroll ... I have left out the deprecated messages see the parent issue as I think we are still to confirm acceptable text...

martin107’s picture

Just making notes for the next iteration

ModulesUninstallForm -- was only partially converted

$this->schema needs to become $this->schemaInstaller

And also

I was able to make 2 methods on SchemaInstaller private --- which is a good sign in terms of decoupling .. hiding away things that should be internal.

Status: Needs review » Needs work

The last submitted patch, 4: 2908886-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

martin107’s picture

Title: Convert schema.inc to a service ( extension function calls ) » Split off SchemaInstaller from schema.inc
Status: Needs work » Needs review
FileSize
4.69 KB
39.1 KB

A) Fixed errors described in #5

B) I think the new installer can be lazy loaded.

Status: Needs review » Needs work

The last submitted patch, 7: 2908886-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

Assigned: martin107 » andypost
Status: Needs work » Needs review
FileSize
39.08 KB

Reroll

Status: Needs review » Needs work

The last submitted patch, 10: 2908886-10.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

andypost’s picture

Status: Needs work » Needs review
FileSize
17.64 KB
45.47 KB

Added proxyclass should fix most of failures
Added some clean-up and removed function back
Replaced remaining usage of drupal_get_installed_schema_version()

Status: Needs review » Needs work

The last submitted patch, 12: 2908886-12.patch, failed testing. View results

andypost’s picture

Status: Needs work » Needs review
Related issues: +#1577902: [META] Remove all usages of drupal_static() & drupal_static_reset()
FileSize
9.02 KB
51.77 KB

Rebase after #2975081: UpdatePathTestBase fails to re-initialize the test site (rebuild container, clear caches) after running the database updates
- Added deprecation messages
- clean-up static usage
WIP cleaning-up usage of drupal_get_module_schema()

not sure about namespace for the service but it looks fine now
The #2124069: Convert schema.inc to the update.update_hook_registry service (UpdateHookRegistry) using
- Drupal\Core\Schema\SchemaData and schema.data service name
- here Drupal\Core\Extension\SchemaInstaller which looks more bound to module installer

andypost’s picture

Assigned: andypost » Unassigned
Issue summary: View changes
Issue tags: +Needs change record updates
FileSize
19.32 KB
62.49 KB

Pointed to separate CR dgo.to/2908886 which needs update after service polished, I don't like the static cache and a way it cleaned, most of services using reset() or resetCache() as separate method
Also third argument mostly unused and could be replaced by separate method to retrieve all schemas

Patch adds a bit of clean-up
Replaced drupal_get_module_schema() and made this method public, otoh it is used by tests & node.install so probably it should be in other helper

andypost’s picture

Issue tags: +Needs tests

and it needs tests for new service when it will be polished + legacy tests

Status: Needs review » Needs work

The last submitted patch, 15: 2908886-15.patch, failed testing. View results

andypost’s picture

Status: Needs work » Needs review
FileSize
1.53 KB
62.69 KB

Fix proxy & status test typo

Status: Needs review » Needs work

The last submitted patch, 18: 2908886-18.patch, failed testing. View results

andypost’s picture

Status: Needs work » Needs review
FileSize
736 bytes
62.75 KB

Trying to fix test, guess static cache of service

Status: Needs review » Needs work

The last submitted patch, 20: 2908886-20.patch, failed testing. View results

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

Status: Needs work » Needs review
FileSize
976 bytes
62.81 KB

So failed test shows that core will not be able to retrieve schema for modules which are not installed
Sadly this test in simpletest module... probably makes sense to move it to core

andypost’s picture

Updated messages for 8.8 still makes sense to split getInstalledVersion() into method that returns array of all schemas and the method to return installed schema

voleger’s picture

Have a few comments here:

  1. +++ b/core/includes/schema.inc
    @@ -70,28 +75,19 @@ function drupal_get_schema_versions($module) {
    - *   The currently installed schema version, or SCHEMA_UNINSTALLED if the
    - *   module is not installed.
    + *   The currently installed schema version, or
    + *   \Drupal\Core\Extension\SchemaInstallerInterface::UNINSTALLED if the module
    + *   is not installed.
    + *
    + * @deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. Use
    + *   \Drupal\Core\Extension\SchemaInstallerInterface::getInstalledVersion()
    + *   instead.
    + *
    + * @see https://www.drupal.org/node/2970993
    

    We can move \Drupal\Core\Extension\SchemaInstallerInterface::UNINSTALLED into @see annotation and leave just SchemaInstallerInterface::UNINSTALLED in @return annotation description. This should increase the readability of the comment.

  2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -59,14 +66,20 @@ class ModuleInstaller implements ModuleInstallerInterface {
    -  public function __construct($root, ModuleHandlerInterface $module_handler, DrupalKernelInterface $kernel) {
    +  public function __construct($root, ModuleHandlerInterface $module_handler, DrupalKernelInterface $kernel, SchemaInstallerInterface $schema_installer = NULL) {
         $this->root = $root;
         $this->moduleHandler = $module_handler;
         $this->kernel = $kernel;
    +    if (!$schema_installer) {
    +      $schema_installer = \Drupal::service('schema_installer');
    +    }
    +    $this->schemaInstaller = $schema_installer;
       }
    

    should we mention here that $schema_installer parameter will be required in the 9.0.0 version? or will we leave that as it is?

  3. +++ b/core/lib/Drupal/Core/Extension/SchemaInstaller.php
    @@ -0,0 +1,206 @@
    +  private function initialize(&$schema, $module, $remove_descriptions = TRUE) {
    

    Type hint "array" missing for $schema

martin107’s picture

Assigned: Unassigned » martin107

@voleger - thanks for the review.

I am working on this now.. As I start all the points look reasonable.

martin107’s picture

Assigned: martin107 » Unassigned
FileSize
63.19 KB
4.81 KB

When it came to sorting out #25.2

When I remove the @deprecated tags then the stroke through is removed on my IDE - which I like.

So in the end I just adopted the shortname convention when referring the classes/methods

voleger’s picture

Issue summary: View changes
Status: Needs review » Needs work

This part looks good
But we have Needs tests tag, so at least we need to add legacy tests for those functions.

voleger’s picture

kim.pepper’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record updates, -Needs tests
FileSize
4.33 KB
66.04 KB

Added legacy tests for the deprecated functions, and updated the change record.

I found a major WTF with getInstalledVersion() which takes a flag to return an array of all modules?? Why aren't we just using drupal_get_schema_versions() or adding a new method to return all installed schema versions?

+++ b/core/lib/Drupal/Core/Extension/SchemaInstallerInterface.php
@@ -0,0 +1,90 @@
+  public function getInstalledVersion($module, $reset = FALSE, $array = FALSE);
kim.pepper’s picture

Discussed this issue with @claudiu.cristea at DrupalCon Seattle and agreed rather than getInstalledVersion($module, $reset = FALSE, $array = FALSE) we can split this into three methods:

  • getInstalledVersion($module)
  • getInstalledVersions()
  • resetCache()

I think this makes a cleaner API.

Still need to update deprecation message and CR but thought I'd post a patch here for review.

Status: Needs review » Needs work

The last submitted patch, 31: 2908886-31.patch, failed testing. View results

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
67.83 KB

Seems the above changes didn't take into account initializing the static cache. I've added a protected method to do that.

Status: Needs review » Needs work

The last submitted patch, 33: 2908886-33.patch, failed testing. View results

kim.pepper’s picture

Missed a call to resetCache() in UpdateScriptTest

andypost’s picture

+++ b/core/core.services.yml
@@ -544,6 +544,10 @@ services:
+  schema_installer:

Naming probably better to make inline with #2124069: Convert schema.inc to the update.update_hook_registry service (UpdateHookRegistry)
So kinda database.schema.installer

daffie’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
daffie’s picture

A quick review of the current patch:

  1. +++ b/core/lib/Drupal/Core/Extension/SchemaInstaller.php
    @@ -0,0 +1,226 @@
    +    return isset($this->currentVersions[$module]) ? $this->currentVersions[$module] : static::UNINSTALLED;
    

    Maybe a stupid question, but can we not use the operator ?: here?

  2. +++ b/core/lib/Drupal/Core/Extension/SchemaInstaller.php
    @@ -0,0 +1,226 @@
    +    // Set the name and module key for all tables.
    ...
    +      if (empty($table['module'])) {
    +        $table['module'] = $module;
    +      }
    +      if (!isset($table['name'])) {
    +        $table['name'] = $name;
    +      }
    

    Why are we doing this. When we are removing the descriptions from the schema to improve serialize() and unserialize()?

  3. +++ b/core/modules/simpletest/src/Tests/KernelTestBaseTest.php
    @@ -186,14 +188,15 @@ public function testInstallSchema() {
    -    $schema = drupal_get_module_schema($module, $table);
    -    $this->assertTrue($schema, "'$table' table schema found.");
    +    $schema = $schema_installer->getSchema($module, $table);
    +    // Not installed module returns empty schema.
    +    $this->assertTrue(empty($schema), "'$table' table schema empty.");
    

    This change looks to me as an API change.

  4. +++ b/core/modules/system/tests/src/Functional/Update/UpdateScriptTest.php
    @@ -214,12 +215,16 @@ public function testSuccessfulUpdateFunctionality() {
    +    assert($schema_installer instanceof SchemaInstallerInterface);
    

    Not sure if this kind of testing is allowed?

  5. +++ b/core/tests/Drupal/KernelTests/Core/Extension/SchemaDeprecationTest.php
    @@ -0,0 +1,60 @@
    +    $schema = [];
    +    _drupal_schema_initialize($schema, 'color');
    

    Can we remove the variable $schema.

  6. +++ b/core/lib/Drupal/Core/Extension/SchemaInstallerInterface.php
    @@ -0,0 +1,97 @@
    +  public function getInstalledVersion($module);
    

    Missing 2 parameters in the docblock.

  7. +++ b/core/lib/Drupal/Core/Extension/SchemaInstallerInterface.php
    @@ -0,0 +1,97 @@
    +   * @return string|int|array
    +   *   The currently installed schema version, static::UNINSTALLED if the
    +   *   module is not installed, or an array for all modules.
    

    The return value type looks wrong to me.

  8. +++ b/core/lib/Drupal/Core/Extension/SchemaInstallerInterface.php
    @@ -0,0 +1,97 @@
    +   * Note: This function does not pass the module's schema through
    +   * hook_schema_alter(). The module's tables will be created exactly as the
    +   * module defines them.
    ...
    +   * Note: This function does not pass the module's schema through
    +   * hook_schema_alter(). The module's tables will be deleted exactly as the
    +   * module defines them.
    

    I am not sure if this is what the non-core/contrib developer expects.

vacho’s picture

andypost’s picture

Here's a re-roll + fixes for #38 and refactored deprecation tests to not fail

1) no way (tested on php 7.3) - it reports Undefined index: color for deprecation test
2) no idea why it supposed to work this way but conversion should not change logic (actually this method is never used in core with TRUE as last argument in `install()/uninstall()` of new service)
3) that's strange case - not installed module should not create tables and result should be empty array, no idea how it passed before
4) fixed
5) can't because this array passed by reference (but the test factored)
6) this 2 parameters not needed in new interface
7) it should return string or array, but int is valid as well because we don't do strict type in setter
8) yes, it means that schema should be altered only on read

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

daffie’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

The patch needs a reroll and the deprecations need to be for 9.1.

Suresh Prabhu Parkala’s picture

Assigned: Unassigned » Suresh Prabhu Parkala
Suresh Prabhu Parkala’s picture

Assigned: Suresh Prabhu Parkala » Unassigned
aleevas’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
57.17 KB
45.83 KB

The latest patch was re-rolled for 9.1.x

Status: Needs review » Needs work

The last submitted patch, 46: 2908886-9.1-46.patch, failed testing. View results

andypost’s picture

Status: Needs work » Needs review
FileSize
10.57 KB
59.85 KB

Fix re-roll
- deprecation test been lost
- version string changed to 9.1=>10.0
- meaningful BC was broken

andypost’s picture

FileSize
1.07 KB
60.1 KB

Extend test coverage for drupal_set_installed_schema_version()

The last submitted patch, 48: 2908886-48.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 49: 2908886-49.patch, failed testing. View results

andypost’s picture

Status: Needs work » Needs review
FileSize
7 KB
66.2 KB

Fix all remaining usage

+++ b/core/modules/system/src/Controller/DbUpdateController.php
@@ -106,6 +116,7 @@ public function __construct($root, KeyValueExpirableFactoryInterface $key_value_
+    $this->schemaInstaller = $schema_installer;

+++ b/core/modules/system/src/Form/ModulesUninstallForm.php
@@ -68,12 +77,15 @@ public static function create(ContainerInterface $container) {
+    $this->schemaInstaller = $schema_installer;

this places probably need BC shim but not sure

Status: Needs review » Needs work

The last submitted patch, 52: 2908886-52.patch, failed testing. View results

andypost’s picture

Status: Needs work » Needs review
FileSize
1.39 KB
66.15 KB

Fixed last failure and removed useless arguments

andypost’s picture

FileSize
650 bytes
66.12 KB

A bit more clean-up

andypost’s picture

FileSize
760 bytes
66.07 KB

Fix proxy class with changes #54 - now it's ready for review

PS: form and controller from #52 is a question still

longwave’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -59,14 +66,21 @@ class ModuleInstaller implements ModuleInstallerInterface {
    +      @trigger('Calling ModuleInstaller::constructor() with the $schema_installer argument is supported in Drupal 8.8.0 and will be required in Drupal 9.0.0.  See https://www.drupal.org/project/drupal/issues/2908886', E_USER_DEPRECATED);
    

    Should be @trigger_error() and __construct(), needs updating to Drupal 9.1.0/10.0.0 and do we need to use the "drupal:9.1.0" format?

    It might also be better to say "The schema_installer service must be passed to ModuleInstaller::__construct()" rather than the variable name.

  2. +++ b/core/modules/system/src/Controller/DbUpdateController.php
    @@ -106,6 +116,7 @@ public function __construct($root, KeyValueExpirableFactoryInterface $key_value_
    +    $this->schemaInstaller = $schema_installer;
    

    I think we should add BC layer here.

  3. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
    @@ -68,12 +77,15 @@ public static function create(ContainerInterface $container) {
    +    $this->schemaInstaller = $schema_installer;
    

    and here

andypost’s picture

Status: Needs work » Needs review
FileSize
5.27 KB
66.8 KB

Fixed #57

Looks there's no standard for this BC so this approach looks the most verbose

daffie’s picture

Issue summary: View changes
Status: Needs review » Needs work

Patch looks good.

  1. +++ b/core/lib/Drupal/Core/Extension/SchemaInstallerInterface.php
    @@ -0,0 +1,97 @@
    +   * @return string|int|array
    +   *   The currently installed schema version, static::UNINSTALLED if the
    +   *   module is not installed, or an array for all modules.
    +   */
    +  public function getInstalledVersion($module);
    

    When does this method return all modules?

  2. +++ b/core/lib/Drupal/Core/Extension/SchemaInstaller.php
    @@ -0,0 +1,226 @@
    +    if (!$this->currentVersions = $this->keyValue->get('system.schema')->getAll()) {
    +      $this->currentVersions = [];
    +    }
    

    Are these lines necessary for the resetCache() method? Can they be removed?

  3. +++ b/core/lib/Drupal/Core/Extension/SchemaInstaller.php
    @@ -0,0 +1,226 @@
    +  private function initialize(array &$schema, $module, $remove_descriptions = TRUE) {
    

    Why make this a private method instead of a protected method?

  4. +++ b/core/lib/Drupal/Core/Extension/SchemaInstaller.php
    @@ -0,0 +1,226 @@
    +  /**
    +   * Statically cached schema data.
    +   *
    +   * @var array
    +   */
    +  protected $schema;
    +
    +  /**
    +   * Statically cached complete schema data.
    +   *
    +   * @var array
    +   */
    +  protected $completeSchema;
    +
    +  /**
    +   * A static cache of schema currentVersions per module.
    +   *
    +   * @var array
    +   */
    +  protected $allVersions = [];
    

    Why are those 3 class variables added. They are not used.

  5. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
    @@ -486,12 +487,16 @@ public function testSuccessfulUpdateFunctionality() {
    +    $schema_installer->resetCache();
    

    Can this line be removed. The method SchemaInstaller::setInstalledVersion() already does a resetCache().

  6. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBaseTest.php
    @@ -34,9 +34,11 @@ protected function setDatabaseDumpFiles() {
    -
    +    /** @var \Drupal\Core\Extension\SchemaInstallerInterface $schema_installer */
    +    $schema_installer = \Drupal::service('schema_installer');
         foreach (['user' => 8100, 'node' => 8700, 'system' => 8805, 'update_test_schema' => 8000] as $module => $schema) {
    -      $this->assertEqual(drupal_get_installed_schema_version($module), $schema, new FormattableMarkup('Module @module schema is @schema', ['@module' => $module, '@schema' => $schema]));
    +      $this->assertEqual($schema_installer->getInstalledVersion($module), $schema, new FormattableMarkup('Module @module schema is @schema', ['@module' => $module, '@schema' => $schema]));
    +
    

    The empty line should not be moved.

  7. I found in the method Drupal\KernelTests\KernelTestBase::installSchema() a reference to "drupal_get_module_schema()" in a comment.
andypost’s picture

Status: Needs work » Needs review
FileSize
9.53 KB
67.03 KB

Fix for #59

Meantime while fixing (5) I found that after running upgrades schema needs re-init and only in \Drupal\Tests\system\Functional\UpdateSystem\UpdateScriptTest::testSuccessfulUpdateFunctionality()

So I changed \Drupal\Core\Extension\SchemaInstallerInterface::resetCache() to allow chaining which is used now.

Also renamed initCurrentVersions() to ensureInitialized() as core using mostly this naming

andypost’s picture

daffie’s picture

Issue summary: View changes
Issue tags: -Needs change record updates

Updated the CR and the IS.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

All my points are addressed.
All code changes look good to me.
For me the patch is RTBC.

martin107’s picture

andypost++

thanks for working on this.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for working on this, it would be good to tidy this up. However, I'm not sure about some aspects of the current patch.

Currently, the patch is dealing with (IMO) two things that can and probably should be separate.

1.

::getInstalledVersion()
::getInstalledVersions()
::setInstalledVersion()

These are all based around hook_update_N(), and they apply to modules which may not use hook_schema() at all, but still rely on hook_update_N() (which is an increasing number of modules due to the entity and configuration systems).

2.

::install()
::uninstall()
::getSchema()

These three methods all deal with hook_schema(), installing it when a module is installed, uninstalling it when the module is uninstalled. They are only ever called from ModuleInstaller, so they could potentially be moved to protected methods on that class (with the procedural functions being deprecated with no replacement).

The methods to deal with updates could then stay in the SchemaInstaller service (edit: although, probably renamed) (which would then still need to be used in both the update and module install system, but would be smaller).

Other notes from reviewing:

<ol>

<li>
<code>
+++ b/core/lib/Drupal/Core/Extension/SchemaInstaller.php
@@ -0,0 +1,204 @@
+  public function getInstalledVersion($module) {
+    $this->ensureInitialized();
+    return $this->currentVersions[$module] ?? static::UNINSTALLED;
+  }

Could we add a protected method to get current versions, which handles the ::ensureInitialized() logic too?

  • +++ b/core/lib/Drupal/Core/Extension/SchemaInstaller.php
    @@ -0,0 +1,204 @@
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function resetCache() {
    +    // Setting to NULL to re-read data on first use in ensureInitialized().
    +    $this->currentVersions = NULL;
    +    return $this;
    +  }
    +
    

    We've been trying to move away from ::resetCache() methods, for example in #3047289: Standardize how we implement in-memory caches. It might be worth exploring that here, but if not we should at least open a follow-up I think.

  • +++ b/core/lib/Drupal/Core/Extension/SchemaInstaller.php
    @@ -0,0 +1,204 @@
    +   */
    +  public function setInstalledVersion($module, $version) {
    +    $this->keyValue->get('system.schema')->set($module, $version);
    +    $this->resetCache()->ensureInitialized();
    +  }
    +
    

    Why does this have to call ::ensureInitialized()?

  • martin107’s picture

    Assigned: Unassigned » martin107

    In particular 65.2 is a really good observation...

    I will work on the response to 65

    martin107’s picture

    martin107’s picture

    FileSize
    71.84 KB
    24.21 KB

    I have made a few questionable choices, so best talk about it here.

    1) I was not able to make ModuleInstaller::getSchema() private as I wanted

    the reason module_test.install make use of it.

    Maybe in the morning I will see an elegant way to adjust

    --- a/core/modules/system/tests/modules/module_test/module_test.install
    +++ b/core/modules/system/tests/modules/module_test/module_test.install
    @@ -30,7 +30,7 @@ function module_test_schema() {
      * Implements hook_install().
      */
     function module_test_install() {
    -  $schema = \Drupal::service('schema_installer')->getSchema('module_test', 'module_test');
    +  $schema = \Drupal::service('module_installer')->getSchema('module_test', 'module_test');
       Database::getConnection()->insert('module_test')
         ->fields([
           'data' => $schema['fields']['data']['type'],

    2) The other thing I am less than happy with is the fact that ModuleInstaller has increased in size by over 100 lines

    I have lived with coding standards that rules that files should be no more than 300 lines long...
    and found it useful code smell that something has gotten too complex.

    ModuleInstaller is now 750 lines long .. but on balance I could not find a good way of creating a ModuleHandlerSchemaHelperTrait.
    so I think 750 lines is OK.

    Anyway I am just posting early

    catch’s picture

    module_test.install makes use of it.

    module_test_install() could just call module_test_schema() directly, although it seems to be going through all that work just to get the string 'varchar', in which case I have no idea what it's doing....

    Since it looked a bit weird, I did some archaeology:

    Was originally added here, in 2010, and used to look like this:
    #620298: Schema not available in hook_install()

    +/**
    + * Implements hook_install().
    + */
    +function module_test_install() {
    +  $record = array('data' => 'Data inserted in hook_install()');
    +  drupal_write_record('module_test', $record);
    +}
    

    It was added for this test:

    /**
    +   * Test that calls to drupal_write_record() work during module installation.
    +   *
    +   * This is a useful function to test because modules often use it to insert
    +   * initial data in their database tables when they are being installed or
    +   * enabled. Furthermore, drupal_write_record() relies on the module schema
    +   * information being available, so this also checks that the data from one of
    +   * the module's hook implementations, in particular hook_schema(), is
    +   * properly available during this time. Therefore, this test helps ensure
    +   * that modules are fully functional while Drupal is installing and enabling
    +   * them.
    +   */
    

    drupal_write_record() no longer exists, so the insert is completely unnecessary, I think we can remove it.

    A few things from looking at the patch, trying not to be too nitpicky since I know it's a work in progress:

    1. +++ b/core/core.services.yml
      @@ -560,6 +560,10 @@ services:
           arguments: ['@theme_handler', '@config.factory', '@config.installer', '@module_handler', '@config.manager', '@asset.css.collection_optimizer', '@router.builder', '@logger.channel.default', '@state', '@extension.list.module']
      +  schema_installer:
      +    class: Drupal\Core\Extension\SchemaInstaller
      +    arguments: ['@module_handler', '@cache.default', '@keyvalue', '@cache_tags.invalidator']
      +    lazy: true
      

      We probably need to rename this, although I don't have great ideas. Maybe SchemaVersionUpdater?

    2. +++ b/core/includes/schema.inc
      @@ -152,22 +160,16 @@ function drupal_uninstall_schema($module) {
      -  }
      -  return [];
      +  @trigger_error('drupal_get_module_schema() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use \Drupal\Core\Extension\SchemaInstallerInterface::uninstall() instead. See https://www.drupal.org/node/2970993', E_USER_DEPRECATED);
      +  return \Drupal::service('schema_installer')->getSchema($module, $table);
       }
      

      These will need updating to say there is no replacement.

    3. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
      @@ -602,4 +632,119 @@ public function validateUninstall(array $module_list) {
      +   *   unserialize(). Defaults to TRUE.
      +   */
      +  private function schemaInitialize(array &$schema, $module, $remove_descriptions = TRUE) {
      +    // Set the name and module key for all tables.
      +    foreach ($schema as $name => &$table) {
      +      if (empty($table['module'])) {
      

      We can probably remove support for $remove_descriptions in the new method.

    4. +++ b/core/lib/Drupal/Core/Extension/SchemaInstaller.php
      @@ -0,0 +1,154 @@
      +    $this->moduleHandler = $module_handler;
      +    $this->cacheBackend = $cache_backend;
      +    $this->keyValue = $key_value_factory;
      

      $this->cacheBackend() is unused?

    5. +++ b/core/lib/Drupal/Core/Extension/SchemaInstaller.php
      @@ -0,0 +1,154 @@
      +
      +    if ($cache_tags_invalidator instanceOf Connection )
      +    {
      +      @trigger_error('Passing a \'connections\' parameter to ' . __METHOD__ . ' is deprecated in drupal:9.1.0 and is removed in drupal:10.0.0. See https://www.drupal.org/node/2970993', E_USER_DEPRECATED);
      +      $cache_tags_invalidator = $cache_tags_invalidator_legacy;
      +    }
      

      This seems unnecessary on a new class? Also unused.

    6. +++ b/core/lib/Drupal/Core/Extension/SchemaInstaller.php
      @@ -0,0 +1,154 @@
      +
      +  /**
      +   * {@inheritdoc}
      +   */
      +  public function setInstalledVersion($module, $version) {
      +    $this->keyValue->get('system.schema')->set($module, $version);
      +    $this->resetCache()->ensureInitialized();
      +  }
      

      If this replaced the value in $this->currentVersions(), then we wouldn't need ::resetCache() at all afaict.

    Status: Needs review » Needs work

    The last submitted patch, 68: 2908886-68.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    martin107’s picture

    I am postponing for a short while...

    I want to add modern type checking to the new services.

    Trouble comes when I want to specify a return type of 'void' to a method that will be processed by the proxy builder.

    public function setInstalledVersion(string $module, string $version): void
    

    at the moment the template code insists on always returning something

    I have spun off the side issue above to fix the proxybuilder.

    The patch here is just where I got to when I stopped .. it is not worth a review... I am posting a memory dump.

    Regarding 69... thanks for the review .. it is always appreciated.

    #1 I went with SchameVersionHandler.

    I dropped the Updater .. as I don't want to frighten consumers of the service, who only want to read access to the current state of things.

    I appreciated the criticism of the 'Handler' suffix -- in that "it will only encourage future coders to use it as a dumping ground" .. but I think the "SchemaVersion" part is already very specific so that concern wont arise.

    #2 not yet fixed.

    #3 I agree fixed.

    #4 good much cleaner

    #5 my goof ... thanks for the fix.

    #6 work in progress.

    In short postponed "Not worth running tests"... yet

    andypost’s picture

    Status: Postponed » Needs work
    Related issues: +#3060914: All new Drupal 8/9 code must use strict type hints where possible

    This is not considered yet, also voids could be added in follow up

    andypost’s picture

    Status: Needs work » Needs review
    FileSize
    15.53 KB
    68.5 KB

    Fix #70.6 and make it work

    About naming - it looks more like "schema_registry" as it stores hook_update numbers

    I still think about separation from #65 - in new patches some duplicated code exists (even after clean-up)

    Status: Needs review » Needs work

    The last submitted patch, 73: 2908886-73.patch, failed testing. View results

    andypost’s picture

    Status: Needs work » Needs review
    FileSize
    2.1 KB
    68.48 KB

    Fix tests

    Status: Needs review » Needs work

    The last submitted patch, 75: 2908886-75.patch, failed testing. View results

    daffie’s picture

    Status: Needs work » Needs review
    FileSize
    9.99 KB
    68.59 KB

    Only trying to fix the tests.

    martin107’s picture

    On reflection it was strange to "down tools" so abruptly and work on the side issue.

    daffie and andypost thanks for chasing down the bugs in the mess I left behind, as my focus changed.

    DrupalCommunity++

    I will have more time this weekend to work on this .. but I want to record a conversion with andypost on slack

    A) He advances the idea that we should simplify the task .. split this issue into two

    1) The module installer refactor
    2) The schema registry

    I think this is a good idea.

    B) He suggests we have a redesign of the API

    In particular the needs of contrib and test can we better served by a rethink.

    I will have more time to think about this on the weekend .. but what I see now is that test makes extensive use of ::getSchema()
    in a way could be redesigned

    To repeat andypost concerns from slack

    we need a good mental model outlined for getter/setters and when and when not to cache or reset

    Anyway I hope this captures the conversation

    .... more later.

    andypost’s picture

    @martin107 Thank you a lot posting here!

    Actually the issues already filed (listed in referenced by block)
    - #2124069: Convert schema.inc to the update.update_hook_registry service (UpdateHookRegistry) - could be changed to address module installer part, will care about creating/deleting tables
    - #3012523: Convert the update.inc file functions to a class - should use split as well (single class with static methods looks weird), so could use schema_registry
    - #3130037: system.schema information gets out of sync with module list - to keep in mind to tackle caching

    andypost’s picture

    martin107’s picture

    Thanks for pointing out the other issues... I will help out on those where I can.

    When I look over everything ... there is one aspect of the patch from 77 that is not addressed elsewhere afaics.

    We have too many issues in this area so I just trying an experiment here.

    and that is the encapsulation of drupal_install_schema()/drupal_uninstall_schema() and _drupal_schema_initialize() inside the ModuleInstaller

    LocallyI have tested this patch on a few tests like

    ./vendor/bin/phpunit -c core/ ./core/tests/Drupal/KernelTests/Core/Extension/ModuleInstallerTest.php

    but I want to see what testbot says.

    andypost’s picture

    Here's iteration on this idea
    - renamed methods: getSchemaTables() , installSchema(), uninstallSchema()
    - deprecated functions and added back test from previous patch

    EDIT we can't make this functions as "no-op" to keep BC

    andypost’s picture

    Title: Split off SchemaInstaller from schema.inc » Split off schema management out of schema.inc
    Issue tags: +Needs change record updates, +Needs issue summary update
    FileSize
    13.95 KB
    18.88 KB

    I went ahead and deprecated drupal_get_module_schema() here

    We still need this API - and schema should be always populated because it used in contrib http://grep.xnddx.ru/search?text=drupal_get_module_schema

    Re-title to point split difference with "update registry schema" in #2124069: Convert schema.inc to the update.update_hook_registry service (UpdateHookRegistry)

    I think we should add unit test for the method (meantime extended functional test as there's no unit one yet for module installer)

    dww’s picture

    Status: Needs review » Needs work
    Issue tags: +Kill includes

    Mostly looks good. Obviously nice to remove more from includes. ;)

    Some nits and some concerns of substance:

    1. +++ b/core/includes/schema.inc
      @@ -152,22 +164,16 @@ function drupal_uninstall_schema($module) {
      + *   SchemaVersionHandlerInterface::getSchema() instead.
      

      I believe this needs to be fully qualified class name.

    2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
      @@ -602,4 +617,73 @@ public function validateUninstall(array $module_list) {
      +    // Load the .install file to use hook_schema() implementation.
      +    $this->moduleHandler->loadInclude($module, 'install');
      +    $tables = $this->populateSchemaTables($module);
      ...
      +  private function populateSchemaTables($module) {
      +    $tables = $this->moduleHandler->invoke($module, 'schema') ?? [];
      

      Seems a bit weird to load the include not in the function that cares about the hook_schema() implementation. Shouldn't this loadInclude() happen directly in populateSchemaTables() before we try to invoke()?

    3. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
      @@ -602,4 +617,73 @@ public function validateUninstall(array $module_list) {
      +  private function installSchema($module) {
      ...
      +  private function uninstallSchema($module) {
      

      I should read the whole issue thread, but are we sure we want private for these? ;)

    4. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
      @@ -602,4 +617,73 @@ public function validateUninstall(array $module_list) {
      +   * @return array
      
      +++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
      @@ -81,4 +81,31 @@ public function addUninstallValidator(ModuleUninstallValidatorInterface $uninsta
      +   * @return array
      

      I know core committers tend to push back on array for phpdocs. Maybe this is the best we can do here, since it's a deeply nested array of schema definition. But might be worth surveying other parts of core that deal with this to see if we can do better than this.

    5. +++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
      @@ -81,4 +81,31 @@ public function addUninstallValidator(ModuleUninstallValidatorInterface $uninsta
      +   * It is also used by ::install() and ::uninstall() to ensure that a module's
      

      ::install() isn't a method.

      A) It's installSchema().

      B) We don't have resolution on #2341405: Decide on standard for referencing namespaced classes nor #3112830: [policy, no patch] Allow static::methodName() and/or self::methodName() in @see comments when referring to the same class but I think self::installSchema() is better than a raw :: version.

      Ditto for ::uninstall()

    6. +++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
      @@ -81,4 +81,31 @@ public function addUninstallValidator(ModuleUninstallValidatorInterface $uninsta
      +   * Note: This function does not pass the module's schema through
      +   * hook_schema_alter(). The module's tables will be returned exactly as the
      +   * module defines them.
      

      Looking at the rest of the patch, I'm not seeing where hook_schema_alter() is happening. ;) I'd need to look more closely at the related code. But this seems like it'd be good to have clear comments on.

    7. +++ b/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php
      @@ -99,6 +99,14 @@ public function validateUninstall(array $module_list)
      +        public function getSchema($module, $table = NULL)
      +        {
      +            return $this->lazyLoadItself()->getSchema($module, $table);
      +        }
      

      This is auto-generated, right? That's why it doesn't follow our code style?

    8. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ModuleInstallerTest.php
      @@ -79,11 +79,16 @@ public function testCacheBinCleanup() {
      +    $schema = $module_installer->getSchema('module_test');
      +    $this->assertSame('module_test', $schema['module_test']['module']);
      +    $this->assertSame('module_test', $schema['module_test']['name']);
      

      Is there anything else we can assert for here to make this test coverage more thorough?

    9. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
      @@ -699,7 +699,7 @@ protected function installConfig($modules) {
      +    // Module installer is technically able to install a schema
           // of a non-enabled module, but its ability to load the module's .install
      

      Comment not wrapping to 80 chars.

    andypost’s picture

    Related issues: +#2060629: Remove hook_schema_alter() from core
    FileSize
    1.33 KB
    18.72 KB

    Thank you! btw hook_schema_alter() is gone from d8)

    Feedback to #84 - fixed 1 and 6, thank you for review
    1) fixed
    2) is tricky - public API should ensure that .install is loaded but "private" methods should has already install files loaded, looks it needs better docs
    3) TBD - technically this methods are private but as no agreement we can make them protected with @internal docs
    4) Actual docs in hook_schema() that's why I just referenced it
    5) [un]install() are the service methods which this comment refering to (newly added [un]installSchema() are private)
    6) Removed as no hook in D8
    7) yes, it's generated
    8) that's what populate doing, gonna add unit test to set flow into stone when API is ok
    9) I'm using to keep changes smaller, this comment may need rework depending on 2)

    PS: NW for IS and CR

    dww’s picture

    Thanks for quickly addressing all that!

    1. 🙏
    2.

    is tricky - public API should ensure that .install is loaded but "private" methods should has already install files loaded, looks it needs better docs

    Hrm, I'm not sure why you say this. Seems like loading the .inc file is an implementation detail that the "private" methods care about. If they're invoking that hook to do their work, I'd think it was their responsibility to do whatever it takes to do their job. I don't understand why the implementation of the public method can/should care about this. I don't know of any principle of OO or computing in general that says public methods need to load the code required by their private helpers. Would you be willing to explain your thoughts in more detail so I can better understand your reasoning? Thanks.

    3. 👍
    4. 👍

    5. Oh! Not in the patch, the method already in ModuleInstallerInterface. 🤦‍♂️ Sorry about that, and thanks for clarifying. I still think we want self or static here, not just ::, but maybe it doesn't matter.

    6. 👍
    7. 👍
    8. Sounds good.
    9. Cool, so NW for later, once the docs are finalized. Also sounds good.

    Thanks again,
    -Derek

    andypost’s picture

    trying to elaborate 2)

    1) It's debatable about need for this API at all, but I think it thing of past (when we used drupal_write_record())
    2) what the Schema API is used for in contrib - to get the list of defined tables by hook_schema() (besides of tracking its update numbers) and I bet some custom code needs access to fields (definitions and database schema)
    3) install/uninstall of schema inside of module installer is implementation details (that's why moved to separate methods, private/protected TBD)
    4) the new public method should not widely used (tests and custom from 2) - maybe move it to separate interface, because it looks like "schema definition introspection in installer" (interdiff in #83 shows less then 10 usages in core)
    5) if we introduce this method in context of module installer it should load .install file (because called from contrib/custom) and re-use internal "data gathering" from hook (which may return NULL:) and massaging with module table name each definition .... looks like unit test))

    andypost’s picture

    A bit of clean-up/polishing also making populate method protected and clarify usage for #87

    andypost’s picture

    I came to static class helper for that purpose and no more API changes except deprecation

    Checking the usage it useful to return all tables from schema

    andypost’s picture

    Status: Needs review » Needs work

    The last submitted patch, 90: 2908886-90.patch, failed testing. View results

    andypost’s picture

    andypost’s picture

    Issue summary: View changes
    Issue tags: -Needs issue summary update

    Summary updated

    andypost’s picture

    andypost’s picture

    Related issue using getSpecification() as new method name so ++ to rename it

    catch’s picture

    
    +++ b/core/lib/Drupal/Core/Extension/ModuleSchema.php
    +++ b/core/lib/Drupal/Core/Extension/ModuleSchema.php
    @@ -0,0 +1,34 @@
    

    Apart from the deprecated functions themselves, this is now only used in tests. Could it be a helper method on a test trait instead of a single method class in the core namespace?

    This might mean we need to leave the existing logic in the deprecated functions until we remove them since there wouldn't be anything to call, but it would be less API surface. If a contrib module really needed the same functionality, they could copy the eight lines of code here - but that is something only a schema introspection module would need.

    andypost’s picture

    Moved to \Drupal\KernelTests\Core\Extension\ModuleSchemaTrait::getTablesSpecification()
    I think this namespace better shows its dependency on module installer subsystem

    Reverted old implementation ++ #96 thanks for idea (protected method is best choice, maybe is could be static but...self:: vs static::)

    Also addressed #84.9 - feel free to help rewording

    updated IS with new trait

    Status: Needs review » Needs work

    The last submitted patch, 97: 2908886-97.patch, failed testing. View results

    andypost’s picture

    Status: Needs work » Needs review

    re-queued, unrelated Drupal\Tests\quickedit\FunctionalJavascript\QuickEditFileTest

    andypost’s picture

    Looks name is not needed at all, also module what for?

    +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -602,4 +618,59 @@ protected function populateSchemaTables($module) {
    +    // Set the name and module key for all tables.
    +    foreach ($tables as $name => &$table) {
    +      if (empty($table['module'])) {
    +        $table['module'] = $module;
    ...
    +      if (!isset($table['name'])) {
    +        $table['name'] = $name;
    

    Why this "massaging" is needed? Looks it also remains on drupal_write_record() or early views.

    Can't grep deeper as it was created in #1180112: Move drupal_*_schema_*() functions into one place

    In https://git.drupalcode.org/project/drupal/-/commit/f5032a6cffd17604f9412...

    andypost’s picture

    +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -59,14 +68,21 @@ class ModuleInstaller implements ModuleInstallerInterface {
    -  public function __construct($root, ModuleHandlerInterface $module_handler, DrupalKernelInterface $kernel) {
    +  public function __construct($root, ModuleHandlerInterface $module_handler, DrupalKernelInterface $kernel, Connection $connection = NULL) {
    

    Maybe we can skip this injection for protected method as #2124069: Convert schema.inc to the update.update_hook_registry service (UpdateHookRegistry) will need to add one more dependency?

    catch’s picture

    Regarding #100, it was added in the original 2007 commit, found via

    git log -S "Set the name and module key for all tables."
    

    . 3cafffe63f70f is the commit.

    Given we're moving the logic to protected methods we could go ahead and remove the massaging here.

    andypost’s picture

    As I see from #144765-11: Schema API 1 the "module" key was added to be used for hook_schema_alter() to identify the the module, "name" used only to drop table (probably protection if table name changed by alter

    As we have no schema alter and dwr() - it's ok to remove them, also -1 function call

    catch’s picture

    This looks great to me now.

    kim.pepper’s picture

    Status: Needs review » Reviewed & tested by the community

    Reviewed the patch in detail and it looks good to me.

    martin107’s picture

    Assigned: martin107 » Unassigned
    alexpott’s picture

    Status: Reviewed & tested by the community » Needs work
    1. +++ b/core/includes/schema.inc
      @@ -182,8 +204,14 @@ function drupal_get_module_schema($module, $table = NULL) {
       function _drupal_schema_initialize(&$schema, $module, $remove_descriptions = TRUE) {
      +  @trigger_error('_drupal_schema_initialize() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. No direct replacement is provided. See https://www.drupal.org/node/2970993', E_USER_DEPRECATED);
      

      Are we sure that no db implementation relies on the additional info that's added here?

    2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
      @@ -59,14 +68,21 @@ class ModuleInstaller implements ModuleInstallerInterface {
      -  public function __construct($root, ModuleHandlerInterface $module_handler, DrupalKernelInterface $kernel) {
      +  public function __construct($root, ModuleHandlerInterface $module_handler, DrupalKernelInterface $kernel, Connection $connection = NULL) {
           $this->root = $root;
           $this->moduleHandler = $module_handler;
           $this->kernel = $kernel;
      +    if (!$connection) {
      +      @trigger_error('The database connection must be passed to ' . __METHOD__ . '. Creating ' . __CLASS__ . ' without it is deprecated in drupal:9.1.0 and will be required in drupal:10.0.0. See https://www.drupal.org/node/2970993', E_USER_DEPRECATED);
      +      $connection = \Drupal::service('database');
      +    }
      +    $this->connection = $connection;
      

      We need be careful about injecting services into this class. Ideally we'd introduce an event so we can decouple the dependencies. But the event dispatcher has issues here too - there is an issue to fix this. But if we want to inject this service then we need to refresh it from the container in \Drupal\Core\Extension\ModuleInstaller::updateKernel().

    3. +++ b/core/tests/Drupal/KernelTests/Core/Database/InsertDefaultsTest.php
      @@ -10,6 +11,7 @@
       class InsertDefaultsTest extends DatabaseTestBase {
      +  use ModuleSchemaTrait;
      

      KernelTestBase already uses this trait so I don't think this is necessary.

    4. +++ b/core/tests/Drupal/KernelTests/Core/Database/InsertDefaultsTest.php
      @@ -18,7 +20,7 @@ public function testDefaultInsert() {
      -    $schema = drupal_get_module_schema('database_test', 'test');
      +    $schema = $this->getTablesSpecification(\Drupal::moduleHandler(), 'database_test')['test'];
      
      @@ -52,7 +54,7 @@ public function testDefaultInsertWithFields() {
      -    $schema = drupal_get_module_schema('database_test', 'test');
      +    $schema = $this->getTablesSpecification(\Drupal::moduleHandler(), 'database_test')['test'];
      

      We could just hardcode the default value. It would in some ways be a better test.

    andypost’s picture

    Status: Needs work » Needs review
    Issue tags: +@deprecated
    FileSize
    1.29 KB
    18.55 KB

    Fix for #107 (2 and 3)
    1) yes not used - it just a remains of hook_schema_alter() (history in #100-103)
    4) not sure changing a test makes sense, maybe follow-up?

    andypost’s picture

    I think it needs post update hook to rebuild container, otoh as code has BC it should not be required

    catch’s picture

    The container is keyed to the Drupal core version by default, so any patch release always rebuilds the container with no need for an explicit update.

    +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -597,6 +597,7 @@ protected function updateKernel($module_filenames) {
    +    $this->moduleHandler = $container->get('database');
       }
    

    This needs to be $this->connection doesn't it?

    andypost’s picture

    @catch Thank you! fixed c/p error(

    alexpott’s picture

    @andypost re #107.1 and the answer in #108 - sure it's not used by any core db driver implementation but there are contrib drivers and they might have used these keys. At the very least this needs explaining on the change record.

    Re #107.4

    +++ b/core/tests/Drupal/KernelTests/Core/Database/InsertDefaultsTest.php
    @@ -18,7 +18,7 @@ public function testDefaultInsert() {
    -    $schema = drupal_get_module_schema('database_test', 'test');
    +    $schema = $this->getTablesSpecification(\Drupal::moduleHandler(), 'database_test')['test'];
    
    @@ -52,7 +52,7 @@ public function testDefaultInsertWithFields() {
    -    $schema = drupal_get_module_schema('database_test', 'test');
    +    $schema = $this->getTablesSpecification(\Drupal::moduleHandler(), 'database_test')['test'];
    

    You are already changing the test. There's no need for the schema here - we can hardcode the expectation and be done.

    alexpott’s picture

    Status: Needs review » Needs work
    +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
    @@ -699,21 +701,26 @@ protected function installConfig($modules) {
    +    $specification = $this->getTablesSpecification($module_handler, $module);
    

    I'm not 100% convinced about the need for the trait. Here's why:

    But thinking some more about this...

    +++ b/core/tests/Drupal/KernelTests/Core/Database/InsertDefaultsTest.php
    --- /dev/null
    +++ b/core/tests/Drupal/KernelTests/Core/Extension/ModuleSchemaTrait.php
    
    +++ b/core/tests/Drupal/KernelTests/Core/Extension/ModuleSchemaTrait.php
    +++ b/core/tests/Drupal/KernelTests/Core/Extension/ModuleSchemaTrait.php
    @@ -0,0 +1,36 @@
    
    @@ -0,0 +1,36 @@
    +<?php
    +
    +namespace Drupal\KernelTests\Core\Extension;
    +
    +use Drupal\Core\Extension\ModuleHandlerInterface;
    +
    +/**
    + * Provides methods to access modules' schema.
    + */
    +trait ModuleSchemaTrait {
    

    This is in the wrong location. But it also does not need to be a trait. It can be a public static on a SchemaInspectionHelper in core/tests/Drupal/TestTools - then we don't add new a method to ever KTB and you can get the sharability accross KTB and BTB (and all test types).

    catch’s picture

    But it also does not need to be a trait. It can be a public static on a SchemaInspectionHelper in core/tests/Drupal/TestTools - then we don't add new a method to ever KTB and you can get the sharability accross KTB and BTB (and all test types).

    +1 from me.

    andypost’s picture

    @alexpott thanks for clarification!

    I went with Drupal\TestTools\Extension\SchemaInspector because
    - there's similar "helpers" (Comparator, ClassWriter) around so Helper is too broad
    - schema is only about module extension, but "module" is too narrow

    Version: 9.1.x-dev » 9.2.x-dev

    Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

    andypost’s picture

    longwave’s picture

    Status: Needs review » Needs work

    All deprecations referring to 9.1.0 need to be bumped to 9.2.0. Otherwise this looks ready for commit.

    andypost’s picture

    @longwave thank you, fixed

    andypost’s picture

    Looks CR also needs update because issue is split in 2 but not sure how to deal with it

    longwave’s picture

    Draft CR exists for the other issue: https://www.drupal.org/node/2444417

    So I think we just remove drupal_get_module_schema() from this CR?

    andypost’s picture

    Probably better to split CRs too, so new one for this issue will mention existing when it's done

    The last submitted patch, 117: 2908886-module-schema-117.patch, failed testing. View results

    Status: Needs review » Needs work

    The last submitted patch, 119: 2908886-module-schema-119.patch, failed testing. View results

    andypost’s picture

    Status: Needs work » Needs review
    FileSize
    2.93 KB
    19.25 KB

    Fixed deprecation tests

    daffie’s picture

    daffie’s picture

    Status: Needs review » Needs work

    Patch looks good. Just some minor stuff.

    1. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
      @@ -716,14 +717,20 @@ protected function installConfig($modules) {
      +    // Database connection schema is technically able to create database tables
      

      Maybe better is to change "Database connection schema" to "Database schema service".

    2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
      @@ -606,4 +623,40 @@ public function validateUninstall(array $module_list) {
      +   * @param \Drupal\Core\Database\Schema $schema
      +   *   The database Schema object to create tables.
      ...
      +  protected function installSchema(Schema $schema, $module) {
      ...
      +      $schema->createTable($name, $table);
      

      Why do have the parameter $schema? When have the class variable $this->connection. Is it not easier to do:

      $this->connection->schema()->createTable($name, $table);
      
    3. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
      @@ -606,4 +623,40 @@ public function validateUninstall(array $module_list) {
      +   * @param \Drupal\Core\Database\Schema $schema
      +   *   The database Schema object to drop tables.
      ...
      +  protected function uninstallSchema(Schema $schema, $module) {
      ...
      +        $schema->dropTable($table);
      

      Why do have the parameter $schema? When have the class variable $this->connection. Is it not easier to do:

      $this->connection->schema()->dropTable($table);
      
    4. +++ b/core/tests/Drupal/KernelTests/Core/Database/InsertDefaultsTest.php
      @@ -13,15 +13,14 @@ class InsertDefaultsTest extends DatabaseTestBase {
      +    $this->assertSame('Undefined', $job, 'Default field value is set.');
      
      @@ -45,17 +44,16 @@ public function testDefaultEmptyInsert() {
      +    $this->assertSame('Undefined', $job, 'Default field value is set.');
      

      I think we should use the new SchemaInspector::getTablesSpecification() to get the value "Undefined".

    5. +++ b/core/tests/Drupal/TestTools/Extension/SchemaInspector.php
      @@ -0,0 +1,36 @@
      +class SchemaInspector {
      ...
      +  public static function getTablesSpecification(ModuleHandlerInterface $handler, $module) {
      

      We are adding this testtool new class and method to core. Could we document that in he IS and the CR.

    daffie’s picture

    For #127.2 and #172.3: Create the variable $schema first like: $schema = $this->connection->schema(); instead or reloading the schema service every time.

    longwave’s picture

    Updated the CR to describe the new static helper method: https://www.drupal.org/node/2970993

    One question I spotted while reading the patch to update the CR:

    +++ b/core/modules/system/tests/src/Functional/Module/ModuleTestBase.php
    @@ -59,7 +60,7 @@ public function assertTableCount($base_table, $count = TRUE) {
    +    $tables = array_keys(SchemaInspector::getTablesSpecification(\Drupal::moduleHandler(), $module));
    
    @@ -77,7 +78,7 @@ public function assertModuleTablesExist($module) {
    +    $tables = array_keys(SchemaInspector::getTablesSpecification(\Drupal::moduleHandler(), $module));
    

    Should this use $this->container->get('module_handler') instead of the \Drupal wrapper?

    longwave’s picture

    Issue summary: View changes

    Updated IS.

    @daffie Re #127.4 see opposite opinion in #107.4/#112

    andypost’s picture

    Status: Needs work » Needs review
    FileSize
    3.97 KB
    19.04 KB

    Should this use $this->container->get('module_handler') instead of the \Drupal wrapper?

    in functional test should not

    Here's fix for #127 (2 and 3) ++ to #128

    re #127.1 because schema is not service but "bound to connection"

    Patch also adds type-hints to new methods

    andypost’s picture

    longwave’s picture

    Status: Needs review » Reviewed & tested by the community

    All review points addressed, this looks ready to go.

    • catch committed 535fb84 on 9.2.x
      Issue #2908886 by andypost, martin107, kim.pepper, daffie, aleevas,...
    catch’s picture

    Status: Reviewed & tested by the community » Fixed

    Nearly eight years since #2124069: Convert schema.inc to the update.update_hook_registry service (UpdateHookRegistry) was opened, good that we were able to clean up ModuleInstaller to be more self-contained with this one.

    Committed 535fb84 and pushed to 9.2.x. Thanks!

    Status: Fixed » Closed (fixed)

    Automatically closed - issue fixed for 2 weeks with no activity.

    TR’s picture

    The issue summary says:

    drupal_get_module_schema() - No direct replacement is provided. Test code should use \Drupal\TestTools\Extension\SchemaInspector::getTablesSpecification() for introspection.

    Please see #2820696-13: Allow schema inspection for all tables created with a valid schema. The functionality provided by drupal_get_module_schema() is vital for my use case, which is NOT test code. Removing the functionality is a regression, and the above assumption that it is only needed in tests, or not needed at all, is faulty.

    #2820696: Allow schema inspection for all tables created with a valid schema is an old issue that proposes a solution that is relevant here.