Convert schema.inc to the update.update_hook_registry service (UpdateHookRegistry)

+++ b/core/lib/Drupal/Core/Schema/Schema.php
@@ -0,0 +1,397 @@
+      db_create_table($name, $table);
...
+      if (db_table_exists($table['name'])) {
+        db_drop_table($table['name']);

Can we have the db connection injected?

I didn't have time to do much, but I added a bare unit test and renamed one of the methods on the service (along the lines of "fooGet" to "getFoo").

I'm fine with moving it to another service. Do you have anything in mind? database?

I was just hoping ... we will get rid of drupal_write_record at the end anyway.
This was just a reroll for now.

is this possible to make schema revisionable with that?

Thinking a more about schema at all I see no usable example in core for schema.
Fields and entities is going to be schema-less so schema definitions could be deployed as yml files (with own schema)
so it makes sense to implement a schema plugin manager with plugins of definitions

hook_update_hook_N() should gone as migration latter in the cycle

Just a reroll but still failing, but less than before.

Rerolled.

drupal_write_record is gone, maybe makes this easier

nice! seems it worked, it just complains about the misplaced test.
had some time, and just moved it;) lets see; this should have tests up and running

grr

1. +++ b/core/lib/Drupal/Core/Schema/Schema.php
   @@ -0,0 +1,624 @@
   +class Schema {
   

   Lets add an interface

2. +++ b/core/lib/Drupal/Core/Schema/Schema.php
   @@ -0,0 +1,624 @@
   +   */
   +  const UNINSTALLED = -1;
   ...
   +   */
   +  const INSTALLED = 0;
   

   and move those to the interface?

3. +++ b/core/lib/Drupal/Core/Schema/Schema.php
   @@ -0,0 +1,624 @@
   +  function writeRecord($table, &$record, $primary_keys = array()) {
   

   we can get rid of this :D

4. +++ b/core/lib/Drupal/Core/Schema/Schema.php
   @@ -0,0 +1,624 @@
   +  function getSchemaFieldsSql($table, $prefix = NULL) {
   

   public? protected?

In case this is rerolled:

+++ b/core/lib/Drupal/Core/Schema/Schema.php
@@ -0,0 +1,351 @@
+  function getFieldsSql($table, $prefix = NULL) {

+++ b/core/lib/Drupal/Core/Schema/SchemaInterface.php
@@ -0,0 +1,172 @@
+  function getFieldsSql($table, $prefix = NULL);

nitpick alert: visibility missing

Besides that, i think all we need now is just a change record draft?

+++ b/core/lib/Drupal/Core/Installer/Form/SiteConfigureForm.php
@@ -59,12 +60,15 @@ class SiteConfigureForm extends FormBase {
+   * @param \Drupal\Core\Schema\Schema $schema
...
+  public function __construct(UserStorageInterface $user_storage, StateInterface $state, ModuleHandlerInterface $module_handler, CountryManagerInterface $country_manager, Schema $schema) {

+++ b/core/modules/system/src/Controller/DbUpdateController.php
@@ -69,6 +70,13 @@ class DbUpdateController extends ControllerBase {
+   * @var \Drupal\Core\Schema\Schema

@@ -83,14 +91,17 @@ class DbUpdateController extends ControllerBase {
+   * @param \Drupal\Core\Schema\Schema $schema
...
+  public function __construct(KeyValueExpirableFactoryInterface $key_value_expirable_factory, CacheBackendInterface $cache, StateInterface $state, ModuleHandlerInterface $module_handler, AccountInterface $account, EntityDefinitionUpdateManagerInterface $entity_definition_update_manager, Schema $schema) {

Should we typehint SchemaInterface instead?

1. +++ b/core/includes/schema.inc
   @@ -197,14 +112,11 @@ function drupal_set_installed_schema_version($module, $version) {
   + *
   + * @deprecated Use \Drupal::service('schema')->install().
   
   @@ -222,16 +134,11 @@ function drupal_install_schema($module) {
   + *
   + * @deprecated Use \Drupal::service('schema')->uninstall().
   

   Let's add a description whether this is deprecated as of 8.0.x or 9.0.x

2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -946,6 +948,8 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
   +    /** @var \Drupal\Core\Schema\Schema $schemaInterface */
   +    $schema = \Drupal::service('schema');
   

   oh wow, now we get crazy. An interface with a variable as name :p

3. +++ b/core/lib/Drupal/Core/Schema/Schema.php
   @@ -0,0 +1,351 @@
   +  /**
   +   * A static cache of schema versions per module.
   +   *
   +   * @var array
   +   */
   +  protected $updates;
   ...
   +  /**
   +   * A static cache of current module schema versions.
   +   *
   +   * @var array
   +   */
   +  protected $versions;
   +
   

   The variable names are a bit odd, but I guess this came up from the existing code in HEAD.

4. +++ b/core/lib/Drupal/Core/Schema/Schema.php
   @@ -0,0 +1,351 @@
   +   * The module handler.
   +   *
   +   * @var \Drupal\Core\Extension\ModuleHandlerInterface
   +   */
   +  protected $moduleHandler;
   +
   +  /**
   +   * The cache backend.
   +   *
   +   * @var \Drupal\Core\Cache\CacheBackendInterface
   +   */
   +  protected $cacheBackend;
   +
   +  /**
   +   * The key value factory.
   +   *
   +   * @var \Drupal\Core\KeyValueStore\KeyValueFactory
   +   */
   +  protected $keyValue;
   +
   +  /**
   +   * The database connection.
   +   *
   +   * @var \Drupal\Core\Database\Connection
   +   */
   +  protected $connection;
   

   It is sad how this is not decoupled.

5. +++ b/core/lib/Drupal/Core/Schema/Schema.php
   @@ -0,0 +1,351 @@
   +
   +        $this->moduleHandler->alter('schema', $this->completeSchema);
   +
   

   We were thinking whether we really still need a schema alter hook. The amount of usecases really seems to not existing any longer.

6. +++ b/core/lib/Drupal/Core/Schema/Schema.php
   @@ -0,0 +1,351 @@
   +        // If the schema is empty, avoid saving it: some database engines require
   +        // the schema to perform queries, and this could lead to infinite loops.
   +        if (!empty($this->completeSchema) && ($this->drupalGetBootstrapPhase() == DRUPAL_BOOTSTRAP_FULL)) {
   +          $this->cacheBackend->set('schema', $this->completeSchema, Cache::PERMANENT, array('schema' => TRUE));
   +        }
   

   It feels like a potentially fundamental issue that storing schema cache in DB is a bad idea.

7. +++ b/core/lib/Drupal/Core/Schema/Schema.php
   @@ -0,0 +1,351 @@
   +   */
   +  public function getVersions($module) {
   +    if (!isset($this->updates[$module])) {
   +      $this->updates = array();
   +      foreach ($this->moduleHandler->getModuleList() as $loaded_module => $filename) {
   +        $this->updates[$loaded_module] = array();
   

   To be honest on an abstract level it is odd to have a) hook_schema() which primarily cares about table schema and hook_update_N() which cares about schema but also about arbitrary updates like potentially entity update, config changes and really what not.

8. +++ b/core/lib/Drupal/Core/Schema/Schema.php
   @@ -0,0 +1,351 @@
   +    if (!$this->versions) {
   +      if (!$this->versions = $this->keyValue->get('system.schema')->getAll()) {
   +        $this->versions = array();
   +      }
   +    }
   

   You could use one line of if here. Not sure whether this is really better to understand.

9. +++ b/core/modules/system/src/Controller/DbUpdateController.php
   @@ -69,6 +70,13 @@ class DbUpdateController extends ControllerBase {
   +   * @var \Drupal\Core\Schema\Schema
   

   Can I haz interface?

#6 I'm pretty sure that comment has been inaccurate for a long time.

I think this is good to go now. We have to cleanup the module handler coordinated anyway.

Sorry I'm not sure about this one.

For example https://api.drupal.org/api/drupal/core%21includes%21schema.inc/function/... only has only a couple of calls in core outside of tests. Two of the calls are only clearing caches which I think leaves this one place:

https://api.drupal.org/api/drupal/core%21modules%21views%21src%21Entity%...

We need that dependency information, but I'm not sure drupal_get_schema() is the right place to get it - could we instead figure out which module defines the table in hook_views_data() and get the dependency information from that. If for example module A provides the table, but another, optional, module B provides the hook_views_data(), we're really dependent on the module B (which in turn depends on module A via the module dependency system).

If we think we can fix the views issue, then it's probably dead code. If it's not dead code for contrib then still doesn't feel like the current patch would be quite right.

Didn't check the other methods. Bumping to major since if we do deprecate the functionality as well as just the procedural functions it'd be important to communicate that to contrib.

reroll

Here's a re roll with a minor fix that the testbot should like better

for example module A provides the table, but another, optional, module B provides the hook_views_data(), we're really dependent on the module B (which in turn depends on module A via the module dependency system).

Maybe better to s/$module/$provider in SchemaInterface? this will allow to have Core provider

+++ b/core/lib/Drupal/Core/Schema/SchemaInterface.php
@@ -0,0 +1,172 @@
+   * @param string $module
+   *   A module name.
...
+  public function getVersions($module);

otoh this could became extension

1. +++ b/core/includes/schema.inc
   @@ -55,48 +46,16 @@ function drupal_get_schema($table = NULL, $rebuild = FALSE) {
   + * Returns an array of available schema currentVersions for a module.
   

   What's currentVersions? :D

2. +++ b/core/lib/Drupal/Core/Schema/Schema.php
   @@ -0,0 +1,351 @@
   +   *   (optional) Whether to additionally remove 'description' keys of all tables
   

   Over 80 chars.

3. +++ b/core/lib/Drupal/Core/Schema/Schema.php
   @@ -0,0 +1,351 @@
   +   * Determines if the drupal bootstrap phase is full.
   +   *
   +   * @return bool
   +   */
   +  protected function isFullDrupalBootstrapPhase() {
   +    return drupal_get_bootstrap_phase() == DRUPAL_BOOTSTRAP_FULL;
   +  }
   

   Does this method really need to exist? Its description is kinda.. funny :/

4. +++ b/core/lib/Drupal/Core/Schema/SchemaInterface.php
   @@ -0,0 +1,172 @@
   + */
   +
   +
   +namespace Drupal\Core\Schema;
   

   Extra newline.

5. +++ b/core/lib/Drupal/Core/Schema/SchemaInterface.php
   @@ -0,0 +1,172 @@
   + * Provides an interface for database schema handling.
   

   If this description is accurate, I wonder why this is in a new Drupal\Core\Schema namespace instead of Drupal\Core\Database.

6. +++ b/core/lib/Drupal/Core/Schema/SchemaInterface.php
   @@ -0,0 +1,172 @@
   +   *   If TRUE, the schema will be rebuilt instead of retrieved from the cache.
   

   Missing (optional) and "Defaults to FALSE."

7. +++ b/core/lib/Drupal/Core/Schema/SchemaInterface.php
   @@ -0,0 +1,172 @@
   +   *   If TRUE, the schema will be rebuilt instead of retrieved from the cache.
   

   Same as above.

Also, this issue is tagged with "Kill includes" but schema.inc is not actually removed :(

Also, this issue is tagged with "Kill includes" but schema.inc is not actually removed :(

Well yeah I doubt we can get rid of the actual include files, given of the BC layer at that point.

+++ b/core/lib/Drupal/Core/Schema/SchemaInterface.php
@@ -0,0 +1,172 @@
+interface SchemaInterface {

Do we maybe choose a better name? Can't think of one, just throwing out this idea.

New patch taking into account comments in #59 but not moving SchemaInterface to the Drupal\Core\Database namespace.

Reroll after #2406131: Remove stray merge conflict marker from DbUpdateController

This still needs an issue summary as well as a change record.

Added CR: https://www.drupal.org/node/2444417

Thank you!

I'm wondering a bit whether the schema service should be named database.schema or something like that, but yeah I just throw out the idea for now.

I would like to hear what @Crell thinks about this patch.

I'm pretty sure he'd like it.

A patch that moves a bunch of procedural functions into a nicely injected service? Why would I like that? :-)

Overall +1, but there are still some issues outstanding:

1. +++ b/core/core.services.yml
   @@ -255,6 +255,9 @@ services:
   +  schema:
   +    class: Drupal\Core\Schema\Schema
   +    arguments: ['@module_handler', '@cache.default', '@keyvalue', '@database']
   

   I fear this will get confused with the Schema object in the Database namespace. OTOH, I don't want to call it a *Manager either so maybe we just have to live with it?

2. +++ b/core/includes/schema.inc
   @@ -15,8 +15,10 @@
    /**
     * Indicates that a module has not been installed yet.
   + *
   + * @deprecated Use \Drupal\Core\Schema\SchemaInterface::UNINSTALLED.
     */
   -const SCHEMA_UNINSTALLED = -1;
   +const SCHEMA_UNINSTALLED = \Drupal\Core\Schema\SchemaInterface::UNINSTALLED;
   

   Minor pedantic point: Shouldn't this leverage a "use" statement instead, per coding standards?

3. +++ b/core/lib/Drupal/Core/Schema/Schema.php
   @@ -0,0 +1,342 @@
   +        if ($rebuild) {
   +          Cache::invalidateTags(array('schema'));
   +        }
   

   Wait, this is static? Really, cache system? Is there nothing we can inject instead?

4. +++ b/core/lib/Drupal/Core/Schema/Schema.php
   @@ -0,0 +1,342 @@
   +      // Ensure that updates are applied in numerical order.
   +      foreach ($this->allVersions as &$module_updates) {
   +        sort($module_updates, SORT_NUMERIC);
   +      }
   

   I think this is pre-existing, but eek at using &-format in a foreach(). That's a land mind. If there's no alternative (a basic for loop is fine), then at least explicitly unset $module_updates afterward. There's no bug here yet, since that variable is not reused, but it's a good habit to get into if you must use foreach-&. (Or just don't use foreach-&.)

5. +++ b/core/lib/Drupal/Core/Schema/SchemaInterface.php
   @@ -0,0 +1,175 @@
   +  /**
   +   * Gets the schema definition of a table, or the whole database schema.
   +   *
   +   * The returned schema will include any modifications made by any
   +   * module that implements hook_schema_alter().
   +   *
   +   * @param string $table
   +   *   (optional) The name of the table. Defaults to NULL, which means that the
   +   *   schema of all tables is returned.
   +   * @param bool $rebuild
   +   *   (optional) If TRUE, the schema will be rebuilt instead of retrieved from
   +   *   the cache. Defaults to FALSE.
   +   */
   +  public function get($table = NULL, $rebuild = FALSE);
   

   With getComplete() there's no need for this method to do double-duty. Just make $table required. If someone wants the whole schema they can call getComplete().

   (The legacy procedural function can handle the if-branch for older code.)

6. +++ b/core/lib/Drupal/Core/Schema/SchemaInterface.php
   @@ -0,0 +1,175 @@
   +   * @return array|bool
   +   *   If the module has updates, an array of available updates sorted by
   +   *   version. Otherwise, FALSE.
   +   */
   +  public function getVersions($module);
   

   Probably pre-existing, but array|bool is a stupid return type. If there's no versions available, return an empty array.

   See: http://www.garfieldtech.com/blog/empty-return-values

7. +++ b/core/lib/Drupal/Core/Schema/SchemaInterface.php
   @@ -0,0 +1,175 @@
   +   * 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.
   

   Created? Don't you mean deleted?

8. +++ b/core/lib/Drupal/Core/Schema/SchemaInterface.php
   @@ -0,0 +1,175 @@
   +   * @return array
   +   *   An array of arrays with the following key/value pairs:
   +   *    - success: a boolean indicating whether the query succeeded.
   +   *    - query: the SQL query(s) executed, passed through
   +   *      \Drupal\Component\Utility\String::checkPlain().
   

   It's not clear what the key is. This doesn't tell me enough about the return structure that I could write code against it.

9. +++ b/core/lib/Drupal/Core/Schema/SchemaInterface.php
   @@ -0,0 +1,175 @@
   +   * It is also used by drupal_install_schema() and
   +   * drupal_uninstall_schema() to ensure that a module's tables are
   +   * created exactly as specified without any changes introduced by a
   +   * module that implements hook_schema_alter().
   

   Comment should be updated to reference the methods, no?

#74.1 is exactly why I wanted your opinion on this, nice to see that it's also the first thing you spotted. That probably means we have to try and find a better name for this service/class :/

#74.1 is exactly why I wanted your opinion on this, nice to see that it's also the first thing you spotted. That probably means we have to try and find a better name for this service/class :/

Yeah we kinda all agree here.

Wait, this is static? Really, cache system? Is there nothing we can inject instead?

Let's fix that

Probably pre-existing, but array|bool is a stupid return type. If there's no versions available, return an empty array.

Yeah, good idea to fix it. Its one small DX improvement.

It's not clear what the key is. This doesn't tell me enough about the return structure that I could write code against it.

Neither the old code nor the new code (which is basically the same) actually returned something, so lets drop that piece of documentation.

This time a little bit more sane.

There we go.

Adapted.

+++ b/core/lib/Drupal/Core/Schema/SchemaManager.php
@@ -0,0 +1,351 @@
+  public function getFieldsSql($table, $prefix = NULL) {

Does this need to be public? Seems like it should be an internal implementation detail of the class. Where is it called externally?

Also, dawehner and I are bikeshedding the SchemaManager name in IRC if anyone wants to join. :-P

@crell
... indeed, this method just used for drupal_write_record() and got removed in the meantime.

We talked about stuff and ended up with SchemaData which is at least relative similar to the way how views handles its schema like data.

Ha, I've seen it, but ignored it.

Now I think we're good. Thanks, dawehner and others!

-use Drupal\Core\Database\Database;
+use Drupal\Core\Schema\SchemaDatanterface;
 .
.
  */
-const SCHEMA_UNINSTALLED = -1;
+const SCHEMA_UNINSTALLED = SchemaDatanterface::UNINSTALLED;
.
. 
+  /** @var \Drupal\Core\Schema\SchemaDatanterface $schema */
+  $schema = \Drupal::service('schema.data');
.
.
+use Drupal\Core\Schema\SchemaDatanterface;
. 
.
+   * @var \Drupal\Core\Schema\SchemaDatanterface
+   */
+  protected $schema;
.
.
+namespace Drupal\Core\Schema;
+
+/**
+ * Provides an interface for database schema handling.
+ */
+interface SchemaDatanterface {
+
+  /**
+   * Indicates that a module schema has not been installed yet.
+   */
+  const UNINSTALLED = -1;
+
.
...etc

s/SchemaDatanterface/SchemaDataInterface. Tests pass because of IDE autocomplete :P

CNW for #88

No-one's replied to #50 yet when I last marked this CNW from RTBC.

drupal_get_schema() is used only in tests for itself (was not quite true 5 months ago but is now), and to clear its own cache in case other code uses it (which it never does). So in my opinion that's dead code and we should either leave it where it is with a strong message, or nuke it, but not convert it to a new service.

I didn't check the other methods then and I still haven't now, but I'd be very surprised if this wasn't true for other things too. IMO the service should only include things that we actually use in core or really think should be in there. We could add a new method later as an API addition in a minor release, but we can't take any public methods out of that interface if this patch lands.

Also note that for example field, entity, cache and several other things no-longer even appear in the array returned from drupal_get_schema() any more, so the information you get from that is pretty much useless anyway.

I also wonder a bit how much the install/uninstall schema methods belong here and how much they might live directly in the module installer or a separate service. But that question is better answered when we figure out what here is dead code or not.

catch: So you're suggesting removing this functionality entirely, or nearly so?

I think we should either:

1. Remove it entirely - but see #2447573: [meta] Make sure 8.x - 8.x hook_update_N() testing is possible which wants to use it again

OR

2. Leave it where it is with a strongly worded comment that it's not very useful and/or explicitly deprecate it.

What I don't think we should do is add a new API with that functionality which we'd then have to change again later if we remove it.

hm. If I read that issue correctly, part of the challenge is that we have so many dynamically created tables now that hook_schema (what this code fronts for) is not a reliable source of authority for what tables actually exist, thus we need to either:

1) Remove/Deprecate this code as no longer meaningful.

2) Revamp this code to actually reflect what's in the database, not what's in hook_schema. At which point it's completely different code anyway, and I think that code even exists already in the DB layer. Or if it doesn't, such code should be.

In which case, yes, just remove this code entirely and if there is introspection logic we still need that the Database layer itself doesn't offer, add it. (The linked issue seems like a good test bed to find such introspection needs.)

catch: Sound reasonable to you?

2) Revamp this code to actually reflect what's in the database, not what's in hook_schema. At which point it's completely different code anyway, and I think that code even exists already in the DB layer.

Don't think we do or ever had that unless I'm missing something. If we did have it, we could possibly use it for the field storage update comparison - right now the 'current schema' is stored in state.

You can check if a table, field or index exists, but you can't generate a schema definition automatically given a table.

Also I noticed the schemaapi defgroup in core/lib/Drupal/Core/Database/Schema.php is very outdated.

In Drupal 7 we handled field schema by a dynamic hook_schema() declaration: https://api.drupal.org/api/drupal/modules!field!modules!field_sql_storag... this was explicitly dropped as part of the entity/field schema changes. I don't think we should re-introduce that and it's not really possible with cache where they're responsible for creating their own tables and don't declare a schema to anywhere else.

Hm, you're right, our introspection is minimal right now. Some drivers use(d) schema API for some dynamic queries, which I never liked. I've wanted to add better introspection to the DB layer for a while, but never had the time or impetus. If Entity API and upgradeability provides the impetus, I can see about locating someone with time. (Should be non-BC-breaking as it's just additions; maybe an addition to the interfaces but that affects all of 4 people in the world.)

That seems like a better plan than trying to shore up dynamic hook_schema().

I think the only place that has lived in https://www.drupal.org/project/schema, I considered porting that to core for #1119094: Add a test to verify the schema matches after a database update but it looked like a lot of work, then migrate made that issue less relevant.

Posted more on #2447573: [meta] Make sure 8.x - 8.x hook_update_N() testing is possible - that issue might need splitting into sub-issues one of might then require introspection.

For this issue, I think we should just leave the function where it is, maybe with @deprecated.

We can open a new issue to stop calling it in core and or remove it as 'dead code' but that's technically an API change whereas this issue isn't.

Also still note this likely is not the only function that's dead code that's being converted here, it's just the first/only one I checked.

All the drupal_*_schema_version* functions are still used by update.php. The unprocessed schema getter is used by ModuleInstaller and simpletest
but @catch is totally right, we don't support drupal_get_schema() anymore.

Let's so skip drupal_get_schema() from this patch, kill it with fire in #2451395: drupal_get_schema()/drupal_get_complete_schema() no longer work as expected; remove them and keep the schema service as proposed in earlier versions of the patch.

More fire!!!!

... sometimes its better to go to bed ... we call to a previously private function in drupal_get_compete_schema(),
so let's first get rid of it, see #2451395: drupal_get_schema()/drupal_get_complete_schema() no longer work as expected; remove them

So I agree with the major priority for #2451395: drupal_get_schema()/drupal_get_complete_schema() no longer work as expected; remove them for the reasons described in #50 -- we essentially have an API now that doesn't do what it implies it should. We should evaluate what's left here after that goes in though.

Ok, looks like we can proceed with this now.

A bit of a complicated reroll, so here's an easy review to get the ball rolling again if desired:

--- /dev/null
+++ b/core/lib/Drupal/Core/Schema/SchemaDatanterface.php

Missing an 'I.'

Rerolled.

Added the routebuilder to the ModuleInstaller class.

Next try.

Reverted the changes to core/modules/system/system.install.

+++ b/core/includes/schema.inc
@@ -72,26 +56,12 @@ function drupal_get_schema_versions($module) {
+ * @deprecated in Drupal 8.0.x, will be removed before Drupal 9.

8.2.x - IIRC

Changed deprecated 8.0.x to 8.2.x

+++ b/core/includes/schema.inc
@@ -12,8 +14,10 @@
+ * @deprecated Use \Drupal\Core\Schema\SchemaDataInterface::UNINSTALLED.

Needs version info.

Also, phpcs says this:

FILE: ...ul/pj2/drupal/core/lib/Drupal/Core/Extension/ModuleInstaller.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 75 | ERROR | [x] Parameter comment indentation must be 3 spaces, found 2
    |       |     spaces
    |       |     (Drupal.Commenting.FunctionComment.ParamCommentIndentation)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...Users/paul/pj2/drupal/core/lib/Drupal/Core/Schema/SchemaData.php
----------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
----------------------------------------------------------------------
  3 | ERROR   | [x] Namespaced classes, interfaces and traits should not
    |         |     begin with a file doc comment
    |         |     (Drupal.Commenting.FileComment.NamespaceNoFileDoc)
 10 | WARNING | [x] Unused use statement
    |         |     (Drupal.Classes.UnusedUseStatement.UnusedUse)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...l/pj2/drupal/core/lib/Drupal/Core/Schema/SchemaDataInterface.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 3 | ERROR | [x] Namespaced classes, interfaces and traits should not
   |       |     begin with a file doc comment
   |       |     (Drupal.Commenting.FileComment.NamespaceNoFileDoc)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...drupal/core/modules/system/src/Controller/DbUpdateController.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 108 | ERROR | [x] Parameter comment indentation must be 3 spaces, found
     |       |     2 spaces
     |       |     (Drupal.Commenting.FunctionComment.ParamCommentIndentation)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...pj2/drupal/core/modules/system/src/Form/ModulesUninstallForm.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 68 | ERROR | [x] Parameter comment indentation must be 3 spaces, found 2
    |       |     spaces
    |       |     (Drupal.Commenting.FunctionComment.ParamCommentIndentation)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Thanks Mile23: fixed your points.

Just saw a new usage of drupal_schema_get_field_value() on an issue which reminded me of this one.

1. +++ b/core/includes/schema.inc
   @@ -72,26 +57,12 @@ function drupal_get_schema_versions($module) {
    function drupal_get_installed_schema_version($module, $reset = FALSE, $array = FALSE) {
   

   This should be in the extension system somewhere, it's got nothing to do with the schema API as such.

2. +++ b/core/includes/schema.inc
   @@ -101,11 +72,12 @@ function drupal_get_installed_schema_version($module, $reset = FALSE, $array = F
    function drupal_set_installed_schema_version($module, $version) {
   

   Same here.

3. +++ b/core/includes/schema.inc
   @@ -113,14 +85,12 @@ function drupal_set_installed_schema_version($module, $version) {
    function drupal_install_schema($module) {
   

   Same here.

4. +++ b/core/includes/schema.inc
   @@ -128,16 +98,12 @@ function drupal_install_schema($module) {
    function drupal_uninstall_schema($module) {
   

   Same here.

5. +++ b/core/includes/schema.inc
   @@ -215,21 +150,12 @@ function _drupal_schema_initialize(&$schema, $module, $remove_descriptions = TRU
     */
    function drupal_schema_get_field_value(array $info, $value) {
   -  // Preserve legal NULL values.
   -  if (isset($value) || !empty($info['not null'])) {
   -    if ($info['type'] == 'int' || $info['type'] == 'serial') {
   -      $value = (int) $value;
   -    }
   -    elseif ($info['type'] == 'float') {
   -      $value = (float) $value;
   -    }
   -    elseif (!is_array($value)) {
   -      $value = (string) $value;
   -    }
   -  }
   -  return $value;
   +  return \Drupal::service('schema.data')->getFieldValue($info, $value);
    }
    
   

   This isn't related to install/uninstall, it could be a static method on a utility class maybe? Either way it shouldn't be on the install/uninstall-related methods.

Does anyone watching this issue know if we have one to convert the actual guts/return of hook_schema to something that is easier to remember, i.e like the builder pattern we have for field definitions?

$table = SchemaTable::create('description goes here');
$table->addField('some_field', IntegerSchemaField::create()->setDefault(0)->setNotNull());

Add ArrayAccess to give it a BC layer.

If not I'll add one - cause I can never remember the magic array keys.

Opened #2908735: [meta] Add value objects to represent hook_schema for #123

Here is a reroll....

all of the changes come from the fact that core has removed all instances of array for [].

While I did it I made notes on what changes to include next...

1) When we say deprecated in 8.2.x .. the issue is so old we now mean 8.5.x

2) as stated in #120 and the issue summary ... yes we seem to have gone too far ... things that belong in extension belong in a separate issue.

I will spend a little time fixing this up next ...

1) Correcting a screw-up of mine from the reroll my phpcs.local.yml file got mixed in... removed.

2) As I mention in #125 we now deprecating in 8.5.x

3) Removed changes to functions which are out of scope. ( which should resolve all the point raised in #120 )

So tasks remaining answer #123 ... my focus is shifting to #2908886: Split off schema management out of schema.inc before I forget.

Just making notes, as I update the issue summary

The following functions have been refactored away

drupal_get_complete_schema() --- and yet it is still talked about in the comments.
drupal_get_schema() -- mentioned in the comments and called but not defined ( see dump-database-d6/7.sh )
drupal_get_schema_unprocessed() -- refactored away no followup.

1. +++ b/core/includes/schema.inc
   @@ -12,8 +14,11 @@
   + * @deprecated in Drupal 8.2.x, will be removed before Drupal 9.
   

   Still refs 8.2.x :)

2. +++ b/core/includes/schema.inc
   @@ -24,38 +29,18 @@
   + * @deprecated in Drupal 8.5.x, will be removed before Drupal 9.
   

   We need to add a change record, and then add an @see to that (applies to all of the @deprecated tags)

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -997,7 +997,7 @@ protected function mapToStorageRecord(ContentEntityInterface $entity, $table_nam
   -        $value = drupal_schema_get_field_value($definition->getSchema()['columns'][$column_name], $value);
   +        $value = \Drupal::service('schema.data')->getFieldValue($definition->getSchema()['columns'][$column_name], $value);
   

   We can inject this, but it would most likely be a BC break on the constructor, so probably would require setter injection. Would require a setter and then in the createInstance factory method, call the setter (entity handlers use a createInstance factory method, not create).

2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -39,6 +41,18 @@ class ModuleInstaller implements ModuleInstallerInterface {
   +   * @var \Drupal\Core\Routing\RouteBuilderInterface
   +   */
   +  protected $routeBuilder;
   
   @@ -54,14 +68,21 @@ class ModuleInstaller implements ModuleInstallerInterface {
   +   * @param \Drupal\Core\Routing\RouteBuilderInterface $route_builder
   +   *   (optional) The route builder service to rebuild the routes if a theme is
   +   *   installed.
   ...
   +    $this->routeBuilder = $route_builder;
   

   These changes look unrelated - bad re-roll?

3. +++ b/core/lib/Drupal/Core/Schema/SchemaData.php
   @@ -0,0 +1,267 @@
   +      $this->allVersions[$module] = [];
   +
   +      foreach ($this->moduleHandler->getModuleList() as $loaded_module => $filename) {
   +        $this->allVersions[$loaded_module] = [];
   +      }
   

   we could use array_map here instead of looping and avoid the initialisation call too?

   $this->allVersions = array_map(function () {
     return [];
   }, $this->moduleHandler->getModuleList());
   

   micro-optimisation though - but I don't think this is preexisting code, so figure in scope

4. +++ b/core/lib/Drupal/Core/Schema/SchemaData.php
   @@ -0,0 +1,267 @@
   +    else {
   +      return isset($this->currentVersions[$module]) ? $this->currentVersions[$module] : static::UNINSTALLED;
   +    }
   

   no need for the else, the if returned already, although could be pre-existing code - if so - follow up

5. +++ b/core/lib/Drupal/Core/Schema/SchemaData.php
   @@ -0,0 +1,267 @@
   +    // Reset the static cache of module schema currentVersions.
   +    $this->getInstalledVersion(NULL, TRUE);
   

   nice

6. +++ b/core/lib/Drupal/Core/Schema/SchemaData.php
   @@ -0,0 +1,267 @@
   +    elseif (!empty($schema)) {
   

   same, this can be an if, not an elseif, the previous if returned in both code paths - but if existing code, again out of scope (follow ups for both)

7. +++ b/core/lib/Drupal/Core/Schema/SchemaData.php
   @@ -0,0 +1,267 @@
   +      if ($info['type'] == 'int' || $info['type'] == 'serial') {
   ...
   +      elseif ($info['type'] == 'float') {
   

   nit: should use === here, we expect strings. Again...might be existing code though - if so, follow up

8. +++ b/core/lib/Drupal/Core/Updater/Module.php
   @@ -88,7 +88,7 @@ public function getSchemaUpdates() {
   -    if (!$updates = drupal_get_schema_versions($this->name)) {
   +    if (!$updates = \Drupal::service('schema.data')->getVersions($this->name)) {
   

   oh wow, never seen these classes before. These are like a mix of OO and info hooks that aren't plugins. We should create a follow up to explore moving them to plugins.

9. +++ b/core/modules/system/tests/src/Functional/Module/InstallTest.php
   @@ -44,9 +44,9 @@ public function testEnableUserTwice() {
   +    $version = \Drupal::service('schema.data')->getInstalledVersion('system', TRUE);
   ...
   +    $version = \Drupal::service('schema.data')->getInstalledVersion('user', TRUE);
   

   we could put this in a local variable here

10. +++ b/core/modules/system/tests/src/Functional/System/StatusTest.php
    @@ -76,12 +76,12 @@ public function testStatusPage() {
    +    \Drupal::service('schema.data')->setInstalledVersion('update_test_postupdate', 8000);
    ...
    +    \Drupal::service('schema.data')->setInstalledVersion('update_test_postupdate', 8001);
    

    same

11. +++ b/core/modules/system/tests/src/Functional/Update/UpdateScriptTest.php
    @@ -112,7 +112,7 @@ public function testRequirements() {
    +    \Drupal::service('schema.data')->setInstalledVersion('update_script_test', \Drupal::service('schema.data')->getInstalledVersion('update_script_test') - 1);
    
    @@ -195,12 +195,12 @@ public function testSuccessfulUpdateFunctionality() {
    +    $schema_version = \Drupal::service('schema.data')->getInstalledVersion('update_script_test', TRUE);
    ...
    +    \Drupal::service('schema.data')->setInstalledVersion('update_script_test', $schema_version - 1);
    +    $schema_version = \Drupal::service('schema.data')->getInstalledVersion('update_script_test', TRUE);
    
    @@ -254,12 +254,12 @@ public function testSuccessfulMultilingualUpdateFunctionality() {
    +    $schema_version = \Drupal::service('schema.data')->getInstalledVersion('update_script_test', TRUE);
    ...
    +    \Drupal::service('schema.data')->setInstalledVersion('update_script_test', $schema_version - 1);
    +    $schema_version = \Drupal::service('schema.data')->getInstalledVersion('update_script_test', TRUE);
    

    same

12. +++ b/core/modules/system/tests/src/Functional/Update/UpdateScriptTest.php
    @@ -290,12 +290,13 @@ public function testSuccessfulMultilingualUpdateFunctionality() {
    +    $schema_version = \Drupal::service('schema.data')->getInstalledVersion('update_script_test');
    ...
    +    \Drupal::service('schema.data')->setInstalledVersion('update_script_test', $schema_version - 1);
    +    $schema_version = \Drupal::service('schema.data')->getInstalledVersion('update_script_test', TRUE);
    

    same

13. +++ b/core/modules/system/tests/src/Functional/Update/UpdatesWith7xTest.php
    @@ -39,11 +39,11 @@ protected function setUp() {
    +    $this->assertEqual(\Drupal::service('schema.data')->getInstalledVersion('update_test_with_7x'), 8000);
    ...
    +    \Drupal::service('schema.data')->setInstalledVersion('update_test_with_7x', 7110);
    
    +++ b/core/modules/system/tests/src/Kernel/Extension/ModuleHandlerTest.php
    @@ -142,7 +142,7 @@ public function testDependencyResolution() {
    +      $this->assertEqual(\Drupal::service('schema.data')->getInstalledVersion($module), SCHEMA_UNINSTALLED, "$module module was uninstalled.");
    
    @@ -193,7 +193,7 @@ public function testUninstallProfileDependency() {
    +    $this->assertEqual(\Drupal::service('schema.data')->getInstalledVersion($dependency), SCHEMA_UNINSTALLED, "$dependency module was uninstalled.");
    

    same (sorry for listing them all, but different files)

We can inject this, but it would most likely be a BC break on the constructor, so probably would require setter injection. Would require a setter and then in the createInstance factory method, call the setter (entity handlers use a createInstance factory method, not create).

Actually, wouldn't require a setter because php scope.

  public static function createInstance(ContainerInterface $container, EntityTypeInterface $entity_type) {
    $return = new static(
      $entity_type,
      $container->get('database'),
      $container->get('entity.manager'),
      $container->get('cache.entity'),
      $container->get('language_manager')
    );
    $return->schema = $container->get('schema.data');
    return $return;
}

Here is an only reroll

I would still like this to go in ... so Reroll.

1. +++ b/core/core.services.yml
   @@ -418,14 +418,15 @@ services:
   +  schema.data:
   

   Doesn't match the CR.

2. +++ b/core/includes/schema.inc
   @@ -24,38 +29,18 @@
   + * @deprecated in Drupal 8.5.x, will be removed before Drupal 9.
   

   Change to 8.6.x, and needs @see for CR. Same for other @deprecated annotations.

We should either add @trigger_error() to deprecated functions or file a follow-up if we think it'll be too disruptive.

It'd also be great to have a basic unit/kernel type test of SchemaData just so we have some sanity checks. I can't find any unit-ish tests of the functions we're deprecating or any tests of the subsystem, though maybe I don't know exactly where to look.

There are lots of unprocessed review comments ....

I think the very first next step is break the patch into two as per the issue summary.

This change is just cutting out "hunks" for one of the following functions

drupal_get_installed_schema_version()
drupal_set_installed_schema_version()
drupal_install_schema()
drupal_uninstall_schema()

There is lots of good work cut out ( about 50 % ) but that is going into the already created side issue.

Thanks, @martin107.

A) [ this covers #128.1, #134.1 and #134.2 ]

Fixed up deprecation notices and triggers - split change record into 2 separate ones.

Just for reference here is the new which belongs to the other issue

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

The original change record now refers to schema.data not schema.manager - but the whole thing needs some TLC which I think is best done as we get close to RTBC.

B) Fixed up some tests.

There is a place to refactor _drupal_schema_initialize() ....
as it is only called from within methods such as drupal_uninstall_schema()
I think the place is within the other issue

Still todo:-

I am going to divert my time to getting the skeleton of the other issue into shape ... just so I don't drop some work on the floor.
There is lots of unresolved substance from #128
The whole test thing needs consideration.
I think we need to whitelist some deprecation notices... but I will let testbot speak.
More is coming from me...

The testbot says this 1,043 times:

  1x: drupal_get_schema_versions() is deprecated in Drupal 8.6.x and will be removed before Drupal 9.0.0. Instead, you should \Drupal::service('schema.data')->getFieldValue($info, $value). See https://www.drupal.org/node/2444417.

So....

+++ b/core/includes/schema.inc
@@ -24,38 +31,22 @@
+  @trigger_error('drupal_get_schema_versions() is deprecated in Drupal 8.6.x and will be removed before Drupal 9.0.0. Instead, you should \Drupal::service(\'schema.data\')->getVersions(). See https://www.drupal.org/node/2444417.', E_USER_DEPRECATED);

@@ -215,21 +208,15 @@ function _drupal_schema_initialize(&$schema, $module, $remove_descriptions = TRU
+  @trigger_error('drupal_get_schema_versions() is deprecated in Drupal 8.6.x and will be removed before Drupal 9.0.0. Instead, you should \Drupal::service(\'schema.data\')->getFieldValue($info, $value). See https://www.drupal.org/node/2444417.', E_USER_DEPRECATED);

Yah let's throw the error in a follow-up rather than whitelisting, so we don't go into (more) technical debt. #2959269: [meta] Core should not trigger deprecated code except in tests and during updates

Thanks Mile23, It is awkward to carry on a conversation from september 2017.. but #128.3 and #129

the BC issue

Every \Drupal::service() type call in SqlContentEntityStorage and SqlContentEntityschema has a @todo next to it saying "must directly inject" ...

for example #2332857: Decide how secondary entity handlers (like storage_schema) should be instantiated

If it is a BC change then I favour deprecating the whole service and inventing a new name ... given the amount of outstanding @todos this could be very silly .. similarNameOne and then similarNameTwo is just bad.

So my response is to just directly inject here.. then depending on the reviews create a "one ring to bind them all issue" so that we can encapsulate the concerns from many issues and invent a new name for the service, just once.... I hope that does not ruffle anyones feathers.

If there is a BC change ... then WebForm is likely to be broken.

I am not finished yet... I am at the point at which I want to see testbot has to say....

Yah I'll just stay quiet for a while. :-)

Just happy we're killing includes.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -160,11 +169,14 @@ public function getFieldStorageDefinitions() {
    *   The cache backend to be used.
    * @param \Drupal\Core\Language\LanguageManagerInterface $language_manager
    *   The language manager.
+   * @param \Drupal\Core\Schema\SchemaDataInterface $schema_data
+   *   The database schema service.
    */
-  public function __construct(EntityTypeInterface $entity_type, Connection $database, EntityManagerInterface $entity_manager, CacheBackendInterface $cache, LanguageManagerInterface $language_manager) {
+  public function __construct(EntityTypeInterface $entity_type, Connection $database, EntityManagerInterface $entity_manager, CacheBackendInterface $cache, LanguageManagerInterface $language_manager, SchemaDataInterface $schema_data) {
     parent::__construct($entity_type, $entity_manager, $cache);
     $this->database = $database;
     $this->languageManager = $language_manager;
+    $this->schemaData = $schema_data;
     $this->initTableLayout();

The trick to avoid breaking subclasses (of which those kind of base classes usually have a lot) is to make the new argument optional and then fall back to \Drupal::service() in the constructor.

Then it won't break anything if not passed in. In the passed we just wrote it as "$schema_data ?: \Drupal::service()", but you might want to do a if/else so you can do a @trigger_error() in the else case.

See also https://drupal.stackexchange.com/questions/260560/why-is-this-property-o...

Thank you both for all the expert advice.

I appreciate that https://www.drupal.org/core/deprecation insists on standardised text ..

in this case deprecating .. means that parameters will not longer be optional and therefore enforced

Please note I have had to deviate from standard text.

Lets see what testbot says to this.