D8: interface Drupal\Core\Database\StatementInterface
D7: DatabaseStatementInterface

Both of these interfaces have some "optional" methods, where the docblock exists, but the method itself is commented out:

  //protected function __construct(Connection $dbh);
  // (this one is a special case, I would say)

  // public function setFetchMode($mode, $a1 = NULL, $a2 = array());
  // public function fetch($mode = NULL, $cursor_orientation = NULL, $cursor_offset = NULL);
  // public function fetchObject();
  // function fetchAll($mode = NULL, $column_index = NULL, array $constructor_arguments);

These always generate IDE warnings if you use

db_select('menu_links')
  -> ...
  ->execute()->fetchAll();

Possible solutions:
- Additional interface that includes those optional methods. (or make it more than one interface)
- Abstract base class where the optional methods are defined, but an exception is thrown if they are used.
- Multiple-type return statement on execute().

E.g.

  /**
   * @return StatementInterface|ExtendedStatementInterface
   */
  function execute();
CommentFileSizeAuthor
#17 2168241-17.patch2.67 KBandypost

Comments

donquixote’s picture

donquixote commented

I ended up doing a LOT of code style and docblock fixes in dbtng.
https://github.com/donquixote/drupal/compare/D8-2168241-1
This also contains some real bug fixes.
PhpStorm really likes it! Only a few cases are left, where it is legitimate to ignore the IDE warnings.

Lesson learn: If we play nice with PHPDoc, the IDE can actually help us find real bugs, not just cosmetic improvements.

I will make the git history a bit more readable, by splitting up the big commit.
I also think some of the changes might go a bit too far. But at least I can get some feedback on those things.

Crell’s picture

Crell commented

Those are listed because we want docblocks on them but they're actually defined higher in the class hierarchy by PHP itself. I forget exactly why we had to do that, frankly... It had to do with PHP 5.1-era PDO developers being sloppy, that's all I remember. :-)

donquixote’s picture

donquixote commented

I split up the changes into smaller commits. I hope this is not too overwhelming.
https://github.com/donquixote/drupal/compare/D8-2168241-3
The smaller commits will allow to omit some of the changes on rebase.

And this is the one where I cover the fetchAll() and friends.
https://github.com/donquixote/drupal/commit/1b52211d3458b6e701fe747aeb61...

donquixote’s picture

donquixote commented

@Crell:
About the PDO stuff. What if we introduce

Drupal\Core\Database\PDO\PDOInterface
Drupal\Core\Database\PDO\PDOStatementInterface

With the exact signature of the respective PDO classes.

Then PDOCompliantStatementInterface can extend PDOStatementInterface and StatementInterface, etc.

EDIT: Although.. we probably don't need all the PDO methods, only some of them. Especially if we want to have statement classes that behave like PDO but don't inherit from PDO, this will be easier if we reduce the PDOStatementInterface to the most relevant methods.

EDIT: Hold on, I think I am running into the same problem that you did years ago. Let's wait until I actually know what I am talking about.

donquixote’s picture

donquixote commented

I managed to re-activate the 4 methods without introducing a new separate interface, and I managed to avoid a conflict with \PDO !
https://github.com/donquixote/drupal/compare/D8-2168241-5

Commit: https://github.com/donquixote/drupal/commit/289eb13ecbfde4e31e1b1fbaeffa...

donquixote’s picture

donquixote commented

New version, where the stuff in Drupal/migrate/Tests/FakeStatement.php is fixed.
https://github.com/donquixote/drupal/compare/D8-2168241-6

Commit: https://github.com/donquixote/drupal/commit/0668938e2409b71e058ada042b3c...

pwolanin’s picture

pwolanin commented

So, this doesn't cover the execute() method?

donquixote’s picture

donquixote commented

@pwolanin:
No idea really :) I think the entire diff is far too big for one issue, so the execute() probably got lost.
And I see that execute() is already covered here. #2226501: Add execute() to the SelectInterface

Here is one re-rolled tag with fixed conflicts. But this does not really fix the problem of doing too much at once.
https://github.com/donquixote/drupal/compare/D8-2168241-8--reroll-and-fi...
I'd say we should split this up into separate issues and do it one step at a time.

donquixote’s picture

donquixote commented

@pwolanin:
No idea really :) I think the entire diff is far too big for one issue, so the execute() probably got lost.
And I see that execute() is already covered here. #2226501: Add execute() to the SelectInterface

Nope, wrong!
The execute() method is contained in both #6 and #8, see
https://github.com/donquixote/drupal/compare/D8-2168241-8--reroll-and-fi...
"Add SelectInterface::execute()"
https://github.com/donquixote/drupal/commit/eda0a53f08302390b0bd4334b9f0...

The difference to the patch in #2226501: Add execute() to the SelectInterface is that it is added at the bottom of SelectInterface, whereas the other patch adds it directly after preExecute(). I suppose the latter is preferable.

I don't care in which order we commit all this, I am open to suggestions.

donquixote’s picture

donquixote commented

One important question that came to me while looking at this:

If a method Statement::execute() both overrides PDOStatement::execute() and implements StatementInterface::execute(), then is {@inheritdoc} really appropriate?

pwolanin’s picture

pwolanin commented
Status: Active » Needs work
Issue tags: +Needs issue summary update

Needs an update of the issue summary. Also, please post a patch even if you make it possible to look at the comparison view too.

I'm not sure which git history you are talking about making readable.

Looking at the last comparison it seems almost all doxygen cleanup? I don't even see any change to the interface?

Maybe you should split out a pure doc fix patch so that can go in and avoid distracting from the original intent of this issue?

donquixote’s picture

donquixote commented

I'm not sure which git history you are talking about making readable.

If I click on the link in #9, I see a series of commits.

Needs an update of the issue summary.

Or we leave this issue with its original intent, and open a new one for the docblock fixes.

donquixote’s picture

donquixote commented

First split-out: #2259947: Minor bug fixes in database system

donquixote’s picture

donquixote commented
Related issues: +#2259979: Docblock fixes in database system, part I, +#2259947: Minor bug fixes in database system

Another split-out: #2259979: Docblock fixes in database system, part I

I'd say get these other issues in first, then continue here or create more separate issues.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

I believe that this other issue I filed a few days ago is a duplicate of this one:
#2287151: StatementInterface docs have a bunch of obsolete stuff in them

However, this issue is impossible to figure out and there isn't even a patch to look at. Please update the issue summary and upload a patch so we can decide if that other one is a duplicate or not. Thanks!

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.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

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new 2168241-17.patch2.67 KB

At least this change is needed for #2572659: Fix 'Drupal.Commenting.InlineComment.SpacingAfter' coding standard

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Hm...

I don't think this patch resolves this issue, but as stated in comment #15, it is difficult to tell because the issue summary is unclear... Probably this patch should be put into a different issue, or else a full patch that fully resolves this issue needs to be made. We don't want multiple patches on the same issue.

Anyway, regarding this patch:

  1. +++ b/core/lib/Drupal/Core/Database/Statement.php
@@ -31,6 +31,26 @@ class Statement extends \PDOStatement implements StatementInterface {
+   * The PDO manual does not document this constructor, but when overriding the
+   * PDOStatement class with a custom without this constructor, PDO will throw
+   * the internal exception/warning:

    In the second line, there is a word missing... "... with a custom without this constructor". Wording could probably be improved too? 

  2. +++ b/core/lib/Drupal/Core/Database/Statement.php
@@ -31,6 +31,26 @@ class Statement extends \PDOStatement implements StatementInterface {
+   * PDO enforces that the access type of this constructor must be protected,
+   * and lastly, it also enforces that a custom PDOStatement interface (like

    lastly? I don't think there is a list that this is the last part of. This wording is therefore a bit confusing to encounter. 

  3. +++ b/core/lib/Drupal/Core/Database/Statement.php
@@ -31,6 +31,26 @@ class Statement extends \PDOStatement implements StatementInterface {
+   * PDO enforces that the access type of this constructor must be protected,
+   * and lastly, it also enforces that a custom PDOStatement interface (like
+   * this) omits the constructor (declaring it results in fatal errors
+   * complaining about "the access type must not be public" if it is public, and
+   * "the access type must be omitted" if it is protected; i.e., conflicting
+   * statements). The access type has to be protected.

    Really... this whole paragraph is overly long.

    Why do we even need any of this information in the doc block anyway? It *may* have been appropriate when it was an interface method, but in a class I don't think it makes much sense to put any of it here. 

  4. +++ b/core/lib/Drupal/Core/Database/Statement.php
@@ -31,6 +31,26 @@ class Statement extends \PDOStatement implements StatementInterface {
+   * "the access type must be omitted" if it is protected; i.e., conflicting

    Do not use i.e. On another issue we are removing all i.e. in comments.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Version: 8.3.x-dev » 8.4.x-dev

Most of the methods were uncommented here: #2521832: Uncomment StatementInterface methods

We still have a sort of prototype __construct() method, which counts as commented code, and makes us unhappy in #2715485: Fix 'Drupal.Commenting.InlineComment.NoSpaceBefore' coding standard

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

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

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.

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.

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.

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.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#3174662: Encapsulate \PDOStatement instead of extending from it

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.

daffie’s picture

daffie commented
Status: Needs review » Needs work

The class Statement is deprecated. Therefore that part of the patch can be removed.
The part of the patch that removes the docblock for the __construct method of the StatementInterface needs to stay in the patch.

daffie’s picture

daffie commented
Issue tags: +Novice
daffie’s picture

daffie commented
Status: Needs work » Closed (outdated)

The removal of the docblock for the __construct method of the StatementInterface wiil be done in #3177660: Remove public properties from StatementInterface implementations.
That makes redundant and therefore I will close the issue.