Remove __construct() from the interface definition.

Problem/Motivation

Having __construct() definition in Interface is bad in that is serve no purpose. At its core, an interface is a contract. There is no implementation attached with an interface and hence there is nothing to initialize and no need for a constructor.

Even worse, you cannot re-use the Interface for a new fancy object, which could need another sort of constructor (e.g. with more parameters, or/and no parameters at all). There is abstract classes for this.

Refactored solution

Move the __construct() definition to the concrete implementations.

Remaining tasks

Decide if we need the __construct() anyway so we can refactor using an abstract class instead of an interface.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sylvain Lecoy’s picture

There is something freaky in that a simple modification from EntityFormControllerInterface result in breaking a test in Layout ????????

Sylvain Lecoy’s picture

entity_form_controller.patch queued for re-testing.

Sylvain Lecoy’s picture

Ok it was a testbot bug.

tim.plunkett’s picture

Category: bug » task
Priority: Major » Normal
Status: Needs review » Needs work
Issue tags: -best practices

Can you provide a single patch?

Also, http://drupal.org/project/issues/search/drupal?issue_tags=best%20practices shows nothing else, I don't think that is a tag in use.

Sylvain Lecoy’s picture

Status: Needs work » Needs review
FileSize
9.96 KB
tim.plunkett’s picture

Thanks! Assuming nothing blows up, this looks good.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -29,7 +29,10 @@
+   * @param $bin

+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.phpundefined
@@ -29,7 +29,10 @@
+   * @param $bin

+++ b/core/lib/Drupal/Core/Cache/NullBackend.phpundefined
@@ -21,7 +21,10 @@
+   * @param $bin

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -104,10 +104,12 @@
+   * @param $entityType

+++ b/core/lib/Drupal/Core/Queue/Memory.phpundefined
@@ -32,7 +32,10 @@
+   * @param $name

+++ b/core/lib/Drupal/Core/Queue/System.phpundefined
@@ -19,7 +19,10 @@
+   * @param $name

I realize you're just moving these, but you might as well add the datatype here (string, in most cases)

Sylvain Lecoy’s picture

Issue tags: +best practices
FileSize
10 KB

Did it !

Also please not remove this tag it being in use from now :)

tstoeckler’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Cache/NullBackend.php
--- a/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -104,10 +104,12 @@
+   * Constructs a new EntityStorageController object.

That is incorrect.

+++ b/core/lib/Drupal/Core/TypedData/Type/TypedData.php
@@ -25,7 +25,12 @@
+   * Creates a typed data object given its definition.

I guess the standard here would be "Constructs" instead of "Creates" and "TypedData" instead of "typed data"

Sylvain Lecoy’s picture

Status: Needs work » Needs review
FileSize
10.01 KB

Could not find any consistent standard in core yet. But I have been using 'Constructs' for this patch.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Great!

chx’s picture

Yes that's a good idea!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

Sylvain Lecoy’s picture

Version: 8.x-dev » 7.x-dev
Status: Closed (fixed) » Needs review

Need backport ?

Sylvain Lecoy’s picture

I have an HTTP error 0 on /comment-upload/js

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

Sylvain Lecoy’s picture

Category: task » bug
David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

It looks like the Drupal 8 patch moved the documentation to the implementing classes, which makes sense to me.

Why doesn't the Drupal 7 patch do the same? That documentation is useful and we don't want it to completely disappear...

Sylvain Lecoy’s picture

Status: Needs work » Needs review
FileSize
2.88 KB

You are right, my bad !

Status: Needs review » Needs work

The last submitted patch, 1813966-remove-construct.patch, failed testing.

Sylvain Lecoy’s picture

Let's try this again.

Sylvain Lecoy’s picture

Status: Needs work » Needs review
Sylvain Lecoy’s picture

Any chances to review this ?

Sylvain Lecoy’s picture

Status: Needs review » Reviewed & tested by the community

I think I fixed the comment in #18 and such a trivial fix would allow me to set this as RTBC ?

tstoeckler’s picture

RTBC++

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

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