Fix 'Drupal.Classes.InterfaceName' coding standard

There is three interfaces what need to be renamed because those don't have the word 'Interface' in their names:

• DatabaseException -> when we rename it it will generate problems, so need to be refactored as well, as i saw there are plenty of places where this interface is used, don't know can we do it before RC1.
• Two test interface under \Drupal\Tests\Component\Utility\ArgumentsResolverTest which are very easy to rename it

As agreed between the mentors at Drupalcon, according to issues to avoid for novices, I am untagging this issue as "Beginner". This issue contains changes across a very wide range of files and might create too many other patches to need to be rerolled at this particular time. This patch has an automated way to be rerolled later so better to implement it after Drupalcon.

nothing to review

FILE: ...ypost/drupal/core/lib/Drupal/Core/Database/DatabaseException.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 13 | WARNING | Interface names should always have the suffix
    |         | "Interface"
----------------------------------------------------------------------


FILE: ...e/tests/Drupal/Tests/Component/Utility/ArgumentsResolverTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
 207 | WARNING | Interface names should always have the suffix
     |         | "Interface"
 213 | WARNING | Interface names should always have the suffix
     |         | "Interface"
----------------------------------------------------------------------

Moving to 8.2.x since this is a code change.

This patch fixes the interface names in ArgumentsResolverTest.php.

It also fixes DatabaseException by renaming it to DatabaseExceptionInterface, which you may now commence arguing over. :-)

We cannot rename interfaces in the Drupal 8 cycle since that would be an API break.

What would be a good way to duplicate the exception to DatabaseExceptionInterface and maintain API compatibility somehow?

interface DatabaseExceptionInterface extends DatabaseException

maybe? What are scenarios where this would break type compatibility?

In the meantime I think we should have // @codingStandardsIgnoreStart + End comments in those files and proceed.

So then the solution at the intersection of not breaking the API vs having coding standards is:

• Deprecate DatabaseException now for removal in Drupal 9.
• Add standards-ignore tags around it.
• Create DatabaseExceptionInterface which extends DatabaseException.
• Replace DatabaseException usages in core with DatabaseExceptionInterface.

I agree with #16 except for

Create DatabaseExceptionInterface which extends DatabaseException.

Let's do it the other way around, so the BC-removal is just deleting the old one. AFAIK that's also how we've handled this elsewhere.

@tstoeckler: that is not possible. If contrib tries to catch the old DatabaseException then the new DatabaseExeceptionInterface will escape that clause because the interface is not a subtype of DatabaseException.

The only way is that the new interface extends the old one, not the other way around.

And even then I'm sceptical if we should really do this and if there are other API breaking special cases that we are not thinking of right now.

As a first step I would prefer to simply mark DatabaseException as // @codingStandardsIgnoreStart and leave a comment that it should be renamed in Drupal 9.

Ahh you're totally right, sorry.

leave a comment that it should be renamed in Drupal 9.

We have a formalized way of doing that, and it's called @deprecated. :-)

This patch:

• Deprecates DatabaseException for removal before Drupal 9.0.0.
• Adds DatabaseExceptionInterface which extends DatabaseException.
• Changes all usages of DatabaseException to DatabaseExceptionInterface.
• Fixes interface names in ArgumentsResolverTest.php
• Adds the coding standard to phpcs.xml.dist

As always: If someone wants to split this up into separate issues that's fine.

The patch fixes all instances of the new rule.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -410,7 +410,7 @@ public function onFieldStorageDefinitionDelete(FieldStorageDefinitionInterface $
     }
-    catch (DatabaseException $e) {
+    catch (DatabaseExceptionInterface $e) {
       // This may happen when changing field storage schema, since we are not

+++ b/core/modules/dblog/src/Logger/DbLog.php
@@ -91,7 +91,7 @@ public function log($level, $message, array $context = array()) {
         // Only handle database related exceptions.
-        ($e instanceof DatabaseException || $e instanceof \PDOException) &&
+        ($e instanceof DatabaseExceptionInterface || $e instanceof \PDOException) &&
         // Avoid an endless loop of re-write attempts.

So I'm wondering about the potential BC break. What happens if you have an exception which throws something that implements DatabaseException, but not DatabaseExceptionInterface. Wouldn't that be a break here? Can't we keep the old exception here for less potential BC breaks?

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -80,7 +80,7 @@ public function query($query, array $args = array(), $options = array()) {
-    catch (DatabaseException $e) {
+    catch (DatabaseExceptionInterface $e) {

this shows the danger of the patch: code that throws DatabaseException will not be caught here.

So I think we should not do this and only ignore the coding standards on the file, leave a @todo to rename it in Drupal 9 and be done with it.

So yeah, let's ignore the DatabaseException file using @codingStandardsIgnoreFile

Attached is a patch to enable the interfacename sniff, fix a few misnamed interfaces and exclude the DatabaseException from the check (but keeping the deprecated tag.)

Looks close to complete, good idea to preserve the @deprecated. I'd like to see the start/end tags changed to just a single "@codingStandardsIgnoreLine". The deprecation notice also should be changed from 8.2.x to 8.4.x now.

Forgot the status change...

I think there is a big difference between patches #20 and #26. Please if I am wrong let me know.
Rerolled 8.2.x again and rebase against 8.4.x. Also I try to fix #27.

@dawehner I keep the new exceptions, maybe I have to change to preserve the old exceptions as you said at #21?

@dimaro, of course there's a big difference between the two patches, because in #22 it was suggested (and in #23 seconded) that the renaming should not take place. I'm not sure which one of these is the correct solution, but as that is the last suggested fix, you should probably comment on the suggestion if you think it's incorrect.

Since the patch in #29 is against the last suggestion, reverting back to Needs work.

1. +++ b/core/lib/Drupal/Core/Database/DatabaseException.php
   @@ -9,5 +10,8 @@
   + *
   + * @deprecated in Drupal 8.4.x for removal before Drupal 9.0.0. Use
   + * Drupal\Core\Database\DatabaseExceptionInterface instead.
     */
   -interface DatabaseException { }
   +interface DatabaseException {}
   

   Can we add a @trigger_error here, see https://www.drupal.org/core/deprecation

2. +++ b/core/lib/Drupal/Core/Database/DatabaseException.php
   @@ -9,5 +10,8 @@
   -interface DatabaseException { }
   +interface DatabaseException {}
   

   This is really tricky. In case some existing code checked for $exception instanceof DatabaseException this will no longer work. To be honest ignoring CS errors here feels like the better solution, better as in not breaking potential existing code.

#29 failed to apply. Re rolled and fixed #32.

Hi,
I rerolled the patch against last dev version.

I don't think we can handle this by ignoring the coding standards and delaying the potentially breaking changes anymore as was suggested in #22 because even future major versions of Drupal now won't be able to make backwards-compatibility breaking changes for features that weren't already deprecated. Thus from the options that have been suggested so far the only possible one I can see for making the change is deprecating the interface and also introducing the new one you are supposed to be using afterwards even if it has the potential of breaking code that uses "instanceof".

Maybe we need some more discussion on whether or not this is the case and if we should go forward with making the risky changes. If this is to be deprecated it will need a change record also.

OK, let's do the following in this issue:

• Add the sniffer interface rule to phpcs.xml.dist.
• Mark DatabaseException to ignore CS.

Then in followups, do the deprecation process:

• Mark DatabaseException as deprecated.
• Create alternative interface.
• Sweep up after.

Attached is a patch containing just the changes for the new issue scope.

Whoops, adding the ArgumentsResolverTest.php fixes back.

Here's a re-roll of #40. I still think that // @codingStandardsIgnoreLine is better than // @codingStandardsIgnoreFile.

Another re-roll.

LGTM

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!