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.

Files: 
CommentFileSizeAuthor
#21 1813966-remove-construct.patch2.37 KBSylvain Lecoy
PASSED: [[SimpleTest]]: [MySQL] 40,480 pass(es).
[ View ]
#19 1813966-remove-construct.patch2.88 KBSylvain Lecoy
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in includes/cache-install.inc.
[ View ]
#9 __construct.patch10.01 KBSylvain Lecoy
PASSED: [[SimpleTest]]: [MySQL] 42,818 pass(es).
[ View ]
#7 __construct.patch10 KBSylvain Lecoy
PASSED: [[SimpleTest]]: [MySQL] 42,800 pass(es).
[ View ]
#5 remove__construct.patch9.96 KBSylvain Lecoy
PASSED: [[SimpleTest]]: [MySQL] 42,681 pass(es).
[ View ]
typed_data.patch1.26 KBSylvain Lecoy
PASSED: [[SimpleTest]]: [MySQL] 42,572 pass(es).
[ View ]
queue.patch1.6 KBSylvain Lecoy
PASSED: [[SimpleTest]]: [MySQL] 42,565 pass(es).
[ View ]
entity_storage_controller_interface.patch1.36 KBSylvain Lecoy
PASSED: [[SimpleTest]]: [MySQL] 42,569 pass(es).
[ View ]
cache.patch2.31 KBSylvain Lecoy
PASSED: [[SimpleTest]]: [MySQL] 42,567 pass(es).
[ View ]
entity_form_controller.patch1.25 KBSylvain Lecoy
PASSED: [[SimpleTest]]: [MySQL] 42,573 pass(es).
[ View ]
archiver.patch2.19 KBSylvain Lecoy
PASSED: [[SimpleTest]]: [MySQL] 42,578 pass(es).
[ View ]

Comments

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

entity_form_controller.patch queued for re-testing.

Ok it was a testbot bug.

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.

Status:Needs work» Needs review
StatusFileSize
new9.96 KB
PASSED: [[SimpleTest]]: [MySQL] 42,681 pass(es).
[ View ]

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)

Issue tags:+best practices
StatusFileSize
new10 KB
PASSED: [[SimpleTest]]: [MySQL] 42,800 pass(es).
[ View ]

Did it !

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

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"

Status:Needs work» Needs review
StatusFileSize
new10.01 KB
PASSED: [[SimpleTest]]: [MySQL] 42,818 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Great!

Yes that's a good idea!

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.

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

Need backport ?

StatusFileSize
new1.75 KB
PASSED: [[SimpleTest]]: [MySQL] 39,874 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Looks good.

Category:task» bug

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...

Status:Needs work» Needs review
StatusFileSize
new2.88 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in includes/cache-install.inc.
[ View ]

You are right, my bad !

Status:Needs review» Needs work

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

StatusFileSize
new2.37 KB
PASSED: [[SimpleTest]]: [MySQL] 40,480 pass(es).
[ View ]

Let's try this again.

Status:Needs work» Needs review

Any chances to review this ?

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 ?

RTBC++

Status:Reviewed & tested by the community» Fixed

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