My client ended up using this module to replace the title field on BEANs which they are using for blocks on their multilingual site. Since the title field isn't required, removing the title (setting it to an empty string) and saving, results in $entity->title = NULL which fails several checks in title.module that use isset().

These checks should be using property_exists() and if they need to, branch for handling NULL values.

CommentFileSizeAuthor
#5 1665006-null_property-5.patch1.14 KBmikey_p
#4 1665006-null_property-4.patch600 bytesmikey_p

Comments

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Issue tags: +Needs tests

I never thought about this use case, since usually labels are required. A patch + tests would be welcome.

Btw, would about !empty() instead of isset()?

mikey_p’s picture

mikey_p commented

empty() won't work either, $foo = NULL, empty($foo) evaluates to TRUE

for example:

php> $obj = new stdClass;
php> $obj->title = NULL;
php> = isset($obj->title);
false
php> = !empty($obj->title);
false
php> = property_exists($obj, 'title');
true
php> = $obj;
<object #7 of type stdClass> {
  title => null,
}
php>
mikey_p’s picture

mikey_p commented
Status: Active » Postponed (maintainer needs more info)

Actually this doesn't seem to be an issue with 7.x-1.x-alpha3.

Waiting to see if this resolves the issue for the client.

mikey_p’s picture

mikey_p commented
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new 1665006-null_property-4.patch600 bytes

Regardless of whether or not the original problem is still manifesting, I think this code is more correct than isset().

mikey_p’s picture

mikey_p commented
StatusFileSize
new 1665006-null_property-5.patch1.14 KB

INCLUDE ALL THE HUNKS.

bojanz’s picture

bojanz commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

Adding this patch to Title in Commerce Kickstart v2.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

Committed and pushed, thanks.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Reviewed & tested by the community » Fixed

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

linking

  • Commit a2f6f70 on 7.x-1.x, workbench authored by mikey_p, committed by plach: 
    Issue #1665006 by mikey_p: Fixed Doesn't work when legacy title field is...