PHP Notice in entity creation form [#2864375]

Problem/Motivation

If a custom entity type is created with the default base controllers, the following notice is displayed when a new entity item creation form page is visited.

Notice: Undefined property: MyEntityClass::$label in Entity->getTranslation() (line 384 of /.../drupal/sites/all/modules/entity/includes/entity.inc).

This happens since entity_label() is called on the page and it assumes the label property is already set for the new entity item being created but actually the property is not set yet unless the developer implements the process to set the label property.

Proposed resolution

Make Entity::defaultLabel() support the state that the label property of the entity object is not set yet.

Remaining tasks

~~Create a patch~~
~~Review the patch~~

User interface changes

(None)

API changes

(None)

Data model changes

(None)

Original report by @dbiscalchin

Everytime I enter the entity form for creation I get the following PHP Notice:

Notice: Undefined property: MyEntityClass::$label in Entity->getTranslation() (line 384 of /.../drupal/sites/all/modules/entity/includes/entity.inc).

which happens because the Entity::getTranslation() method does not check whether the property is set before accessing it.

Steps to reproduce

First, define a custom entity passing the following options on hook_entity_info():

Under "entity keys", set "label" to some property defined on the table schema.
Set "entity class" to "Entity".
Set "entity controller" to "EntityAPIController".
Set "entity_class_label" for "label callback".
Under "admin ui", set "path" for the admin UI to some path of your choice.

Also define other required values and a function to return the entity form as usual.

Then access the chosen path for the Admin UI and click the button to add a new entity.

When accessing the entity form, the notice appears.

At this point, the property was not set yet, because the entity would still be created.

Checking the stack trace, I verified that the function which calls entity_label() is entity_ui_get_page_title().

Comment	File	Size	Author
#10	php_notice_entity_creation_form-2864375-10.patch	648 bytes	dbiscalchin
#10	7.x-1.x: PHP 5.3 & MySQL 5.5, D7 42 pass
#2	php_notice_entity_creation_form-2864375-2.patch	394 bytes	dbiscalchin
#2	7.x-1.x: PHP 5.3 & MySQL 5.5, D7 42 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

27 March 2017 at 14:46

dbiscalchin created an issue. See original summary.

Comment #2

dbiscalchin CreditAttribution: dbiscalchin commented 27 March 2017 at 14:51

Status:

Active

» Needs review

File	Size
php_notice_entity_creation_form-2864375-2.patch	394 bytes
7.x-1.x: PHP 5.3 & MySQL 5.5, D7 42 pass

This is a patch to fix the issue.

Comment #3

hgoto CreditAttribution: hgoto as a volunteer commented 23 July 2017 at 13:57

Category:	Bug report	» Support request
Status:	Needs review	» Needs work
Issue tags:		+Needs steps to reproduce

The patch #2 surely hides the PHP notice. But I'm not sure it fixes the root problem with the provided information.

We need to know why MyEntityClass::$label doesn't exist firstly. Could you share the exact steps to reproduce the problem?

Please change the category back if I'm wrong.

Comment #4

dbiscalchin CreditAttribution: dbiscalchin commented 26 July 2017 at 13:32

Issue summary:

View changes

Comment #5

dbiscalchin CreditAttribution: dbiscalchin commented 26 July 2017 at 14:31

Category:

Support request

» Bug report

Hi @hgoto

I've updated the issue to include the steps to reproduce it. I've tested these steps on a clean Drupal installation.

Also I've changed the category back to "Bug report", because the caller is also an Entity API's function - it is entity_ui_get_page_title(). The solution though might be different if you understand that it should be the client's responsability to call entity_label() only if the property is set. In this case, the verification should be on entity_ui_get_page_title() to not call it if the operation is "add".

Please let me know if you want me to change the patch.

Regards,

Comment #6

dbiscalchin CreditAttribution: dbiscalchin commented 26 July 2017 at 14:28

Issue tags:

-Needs steps to reproduce

Comment #7

hgoto CreditAttribution: hgoto as a volunteer commented 28 July 2017 at 12:13

Thank you. I confirmed the notice occurs. I myself usually prepare a custom entity class instead of using Entity to avoid this kind of problem when using "admin ui". But I agree with you that this is better to be fixed.

As with the solution, IMO it would be better if we have entity_ui_get_page_title() handle the "add" operation more properly as you told as below.

The solution though might be different if you understand that it should be the client's responsability to call entity_label() only if the property is set. In this case, the verification should be on entity_ui_get_page_title() to not call it if the operation is "add".

Comment #8

dbiscalchin CreditAttribution: dbiscalchin commented 3 August 2017 at 18:54

Hi @hgoto

There is also another solution:

We could check if the label property is set on defaultLabel() before calling getTranslation(). I like this approach more because it definitely solves the problem and I think defaultLabel() should not raise a notice if the property is not set but just return NULL instead.

What do you think?

Comment #9

hgoto CreditAttribution: hgoto as a volunteer commented 15 August 2017 at 06:18

Sorry for my late response. Thanks. I see. Please let me organize our thoughts.

I think, if we want to fix only the problem on the entity creation form as the issue summary says, it's enough to change only entity_ui_get_page_title(). entity_ui_get_page_title() looks better for me in that perspective.

On the other hand, if we want to prevent the PHP notice whenever entity_label() is called before the label property is set, it's better to fix Entity::defaultLabel() as you told. This fix means that we think it's not clients' responsibility to set the label before calling entity_label().

Also, if we want to prevent the notice even when Entity::getTranslation() (public method) is directly called, it's better to fix Entity::getTranslation().

So it depends on what is the problem we want to address, doesn't it. I thought it's clients' responsability to set the label properly before calling entity_label() but I don't stick to the idea.

I'm ready to review your patch whichever idea you choose.

Comment #10

dbiscalchin CreditAttribution: dbiscalchin commented 15 August 2017 at 19:07

Status:

Needs work

» Needs review

File	Size
php_notice_entity_creation_form-2864375-10.patch	648 bytes
7.x-1.x: PHP 5.3 & MySQL 5.5, D7 42 pass

Hi again!

Here is my opinion about each alternative:

1) Changing Entity::defaultLabel()
Definitely I think this method shouldn't raise a notice, because it is intended to be a default getter implementation and the developer should rely on it without the need to override it just to check for NULL values.

2) Changing Entity::getTranslation()
In this case, I think its fine to move the responsability to the client as this method requires the property name to be explicitly provided. Moreover, this property must be a string to get a translation of it. Despite this, I think it is fine to return NULL without a notice when the property is not set.

3) Changing entity_ui_get_page_title()
Since I see (1) as a requirement, this alternative wouldn't be applicable. Besides that, if more operations were added to entity_ui_get_page_title(), perhaps more verifications would be necessary.

For these reasons, I prefer alternative (1) which is implemented by the patch I'm sending now.

Thank you!