Clean up DBTNG Schema with Traits

Any particular reason for why we need both abstract methods in the trait AND the same methods on an interface? The interface should be sufficient, no?

Color me not convinced here. Having the interface is a good thing, but your trait here is really an abstract base class... so there is no real advantage in having it as a trait, it just makes things a little bit weirder with no gain.

Traits are for code reuse of relatively self-enclosed behaviors. Here there is no such thing, so let's keep the abstract base class.

Interface yes, trait (as is), no.

Traits are nice when you don't want to squat the only opportunity to use extends that a class can have.
But this is clearly a case where we actually want a base class:
Drupal\Core\Database\Driver\mysql\Schema extends Drupal\Core\Database\Schema

100% agree with #12.

@EclipseGc: it's a question of code organization. ABCs can be considered as vestigial only if you organize your code around small, relatively self contained object behaviors (emphasis on small and self-contained). Without further refactoring we don't do such a thing here, the abstract class Schema is a big mix of a lot of different stuff all closely tied together.

If broken down, many of those would be good fit as a trait (the clearest one being a concrete implementation of PlaceHolderInterface), but until broken down it's better to keep the whole thing as an ABC.

In the interest of progress I'm OK with backing off on the trait here and circling back around later after further refactoring (should any happen in D8), which I think we all agree is long-overdue for Schema API.

Damien is correct that placeholderinterface is a good target for traitification.

The documentation in this patch is not going to work.

The main doc block for this Trait starts with

+/**
+ * @defgroup schemaapi Schema API
+ * @{
+ * API to handle database schemas.
+ *

Defining a @defgroup needs to be separate from the Trait's doc block.

Also, there's an issue about defining standards for how to document traits that may be of interest:
#2206175: [policy, no patch] Figure out how to document traits and trait methods

1. +++ b/core/lib/Drupal/Core/Database/DatabaseSchema.php
   @@ -367,359 +321,119 @@ public function fieldExists($table, $column) {
   -  /**
   -   * Create a new table from a Drupal table definition.
   -   *
   -   * @param $name
   -   *   The name of the table to create.
   -   * @param $table
   -   *   A Schema API table definition array.
   -   *
   -   * @throws \Drupal\Core\Database\SchemaObjectExistsException
   -   *   If the specified table already exists.
   -   */
   -  public function createTable($name, $table) {
   

   It looks like you moved where these methods are in the class (probably an artifact of earlier versions of the patch), which makes the patch larger and harder to review. It took me a while to figure out what happened to all of this code. :-)

   I don't know that there's anything to do here, but I'm mentioning it in case a later reroll could be smaller/easier to read.

2. +++ b/core/lib/Drupal/Core/Database/SchemaInterface.php
   @@ -0,0 +1,423 @@
   +interface SchemaInterface extends PlaceholderInterface {
   

   I'm not sure if the interface should extend, or if the class should just implement both. Open to thoughts here.

I took a look at the documentation changes in this patch. There are a few issues:

a) class DatabaseSchema

   /**
-   * The placeholder counter.
+   * @var \Drupal\Core\Database\Connection
    */
-  protected $placeholder = 0;
+  protected $connection;

A member variable needs to have a one-line description as well as @var. This patch replaces the description with @var instead of adding to the description.

b) DatabaseSchema::getPrefixInfo()

+   * @param string $table
+   *   Name of table to look prefix up for. Defaults to 'default' because that
+   *   is the default key for prefix.

This makes zero sense to me. It says it's the name of the table and then says something about keys and prefixes -- what does this mean? What is a prefix and what is a key? This also applies to several methods in SchemaInterface with 'prefix' in their names. As far as I can see, prefixes are never explained and I have no idea what it is from reading this documentation.

c)

+/**
+ * @see \Drupal\Core\Database\Query\PlaceholderInterface
+ */
+trait PlaceholderTrait {

A Trait still needs to have a one-line description of what it is for. @see just gives you a See Also, not documentation.

d) (minor) In the new SchemaInterface, the method docs should all start with 3rd-person verbs, like "Creates" instead of "Create". Some currently do, and some do not.

These are relatively minor and/or preexisting conditions except (a) and (c), which really needs to be fixed for sure (our Documentation Gate says that everything at least needs one-line documentation).

I think any other missing documentation can/should be handled by #2192185: Improve DB API code documentation. Thanks, Kris!

Looks much better to me.

Looks like we lack a change record draft for this. At least the "use" statement in implementing classes needs to change, not sure how much else of this is developer-facing.

Made a slight language tweak to the change notice. Back to RTBC.

Bot can object.

The failure on #37 looks like the testbot just ran out of diskspace, so nothing to worry about for this issue.

38: 2206183-38.patch queued for re-testing.

Testbot burped. Can we get this committed before it burps again?

+++ b/core/lib/Drupal/Core/Database/SchemaInterface.php
@@ -0,0 +1,423 @@
+   * sequences (from serial-type fields) that use the changed field to be dropped.
...
+   *   New name for the field (set to the same as $field if you don't want to change the name).
...
+   * Return an array of field names from an array of key/index column specifiers.
...
+   * This is usually an identity function but if a key/index uses a column prefix

more then 80 chars.

Updating patch with reroll. Please review.

Not sure this will work, but it should apply.

So if we reroll this again and make it work, when's a good time for it so that it doesn't sit here and get stale again? That's one of the most demoralizing things in the issue queue, especially for something that's just moving code around to make it cleaner...

Is this still under consideration for 8.0.x?