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.
Comment | File | Size | Author |
---|---|---|---|
#21 | 1813966-remove-construct.patch | 2.37 KB | Sylvain Lecoy |
#19 | 1813966-remove-construct.patch | 2.88 KB | Sylvain Lecoy |
#9 | __construct.patch | 10.01 KB | Sylvain Lecoy |
#7 | __construct.patch | 10 KB | Sylvain Lecoy |
#5 | remove__construct.patch | 9.96 KB | Sylvain Lecoy |
Comments
Comment #1
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedThere is something freaky in that a simple modification from EntityFormControllerInterface result in breaking a test in Layout ????????
Comment #2
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedentity_form_controller.patch queued for re-testing.
Comment #3
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedOk it was a testbot bug.
Comment #4
tim.plunkettCan 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.
Comment #5
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedComment #6
tim.plunkettThanks! Assuming nothing blows up, this looks good.
I realize you're just moving these, but you might as well add the datatype here (string, in most cases)
Comment #7
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedDid it !
Also please not remove this tag it being in use from now :)
Comment #8
tstoecklerThat is incorrect.
I guess the standard here would be "Constructs" instead of "Creates" and "TypedData" instead of "typed data"
Comment #9
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedCould not find any consistent standard in core yet. But I have been using 'Constructs' for this patch.
Comment #10
tstoecklerGreat!
Comment #11
chx CreditAttribution: chx commentedYes that's a good idea!
Comment #12
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #14
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedNeed backport ?
Comment #15
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedI have an HTTP error 0 on /comment-upload/js
Comment #16
tstoecklerLooks good.
Comment #17
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedComment #18
David_Rothstein CreditAttribution: David_Rothstein commentedIt 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...
Comment #19
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedYou are right, my bad !
Comment #21
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedLet's try this again.
Comment #22
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedComment #23
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedAny chances to review this ?
Comment #24
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedI think I fixed the comment in #18 and such a trivial fix would allow me to set this as RTBC ?
Comment #25
tstoecklerRTBC++
Comment #26
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/f58bc6a