Currently EntityInterface defines __construct() method with $values parameter without default value

But all implementations uses array $values = array()

Suppose better to make it consistent. phpStorm IDE highlights this as error

CommentFileSizeAuthor
#4 1796604-core-ei-4.patch1.2 KBandypost
core-ei.patch590 bytesandypost

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

No, it needs to be fixed the other way round.

This definition makes no sense. You can't have an optional first argument and a required second.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

The other option would be to switch $values and $entity_type.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Let's fix all implementations first.

Let's discuss whether to flip the arguments in a separate issue.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new 1796604-core-ei-4.patch1.2 KB

New patch to fix implementations
Also filed #1796760: Change order of parameters in Entity constructors

andypost’s picture

andypost
he/him
Primary language Russian
commented
Category: task » bug

This is a actually a bug

podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
commented

#4 looks like very simple catch
do we need separate issue #1796760: Change order of parameters in Entity constructors from this?

andypost’s picture

andypost
he/him
Primary language Russian
commented

@podarok #1796760 is different task, This issue about bug

podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
commented
Status: Needs review » Reviewed & tested by the community

#7 if so
#4 looks good for me
any backgrounds from someone?

xjm’s picture

xjm
she/her
Primary language English
commented

Looks like a good cleanup to me.

chx’s picture

chx commented

Well, certainly this public function __construct(array $values = array(), $entity_type) { makes no sense because if entity_type is mandatory then values is mandatory too. So yes this fix makes sense.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Assigned: Unassigned » jhodgdon
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

Oops. Clicking around too fast. :)

This one is simple enough. Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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