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.

CommentFileSizeAuthor
#21 1813966-remove-construct.patch2.37 KBsylvain lecoy
#19 1813966-remove-construct.patch2.88 KBsylvain lecoy
#15 1813966-remove-construct.patch1.75 KBsylvain lecoy
#9 __construct.patch10.01 KBsylvain lecoy
#7 __construct.patch10 KBsylvain lecoy
#5 remove__construct.patch9.96 KBsylvain lecoy
typed_data.patch1.26 KBsylvain lecoy
queue.patch1.6 KBsylvain lecoy
entity_storage_controller_interface.patch1.36 KBsylvain lecoy
cache.patch2.31 KBsylvain lecoy
entity_form_controller.patch1.25 KBsylvain lecoy
archiver.patch2.19 KBsylvain lecoy

Comments

sylvain lecoy’s picture

sylvain lecoy commented

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

sylvain lecoy’s picture

sylvain lecoy commented

entity_form_controller.patch queued for re-testing.

sylvain lecoy’s picture

sylvain lecoy commented

Ok it was a testbot bug.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
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

sylvain lecoy commented
Status: Needs work » Needs review
StatusFileSize
new remove__construct.patch9.96 KB
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

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

sylvain lecoy commented
Issue tags: +best practices
StatusFileSize
new __construct.patch10 KB

Did it !

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

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
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

sylvain lecoy commented
Status: Needs work » Needs review
StatusFileSize
new __construct.patch10.01 KB

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

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Reviewed & tested by the community

Great!

chx’s picture

chx commented

Yes that's a good idea!

dries’s picture

dries commented
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

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

Need backport ?

sylvain lecoy’s picture

sylvain lecoy commented
StatusFileSize
new 1813966-remove-construct.patch1.75 KB

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

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Reviewed & tested by the community

Looks good.

sylvain lecoy’s picture

sylvain lecoy commented
Category: task » bug
David_Rothstein’s picture

David_Rothstein commented
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

sylvain lecoy commented
Status: Needs work » Needs review
StatusFileSize
new 1813966-remove-construct.patch2.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

sylvain lecoy commented
StatusFileSize
new 1813966-remove-construct.patch2.37 KB

Let's try this again.

sylvain lecoy’s picture

sylvain lecoy commented
Status: Needs work » Needs review
sylvain lecoy’s picture

sylvain lecoy commented

Any chances to review this ?

sylvain lecoy’s picture

sylvain lecoy commented
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

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

RTBC++

David_Rothstein’s picture

David_Rothstein commented
Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/f58bc6a

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