PHP5.4 warnings and Notices [#1921034]

I am current testing PHP 5.4 and found the fixed the following with the attached patch:

Warning: Creating default object from empty value in tvi_load_settings() (line 47 of /sites/all/modules/tvi/includes/tvi.query.inc).
Notice: Undefined property: stdClass::$status in tvi_get_term_info() (line 282 of /sites/all/modules/tvi/tvi.module).
Notice: Undefined property: stdClass::$status in tvi_get_term_info() (line 288 of /sites/all/modules/tvi/tvi.module).
Notice: Undefined property: stdClass::$inherit in tvi_get_term_info() (line 292 of /sites/all/modules/tvi/tvi.module).
Notice: Undefined property: stdClass::$status in tvi_get_term_info() (line 303 of /sites/all/modules/tvi/tvi.module).

Comment	File	Size	Author
	tvi-php54_compat-d7.patch	2.43 KB	NaX

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

DuaelFr

he/him

French

Montpellier, France

CreditAttribution: DuaelFr commented 20 February 2013 at 21:40

Your patch seems to alter a lot of code without relation with the notices that you are reporting.

Comment #2

NaX CreditAttribution: NaX commented 22 February 2013 at 15:06

All the changes are related to the notice. The biggest problem was that tvi_load_settings() would return a object that did not have all the attributes that where being accessed EG: $settings->inherit.

So I removed the ability to optionally return the default object and forced it to always at min return the default object. This way all the object attributes would always be available.

Comment #3

Simon Georges CreditAttribution: Simon Georges commented 24 February 2013 at 18:42

Aren't they the same notices already fixed by #1908618: Notice: Undefined property: stdClass::$inherit in tvi_get_term_info()? @NaX, could you try with the -dev version?

Comment #4

NaX CreditAttribution: NaX commented 25 February 2013 at 12:28

I did not see that patch. Yes, that patch does address some of the same warnings, but not all of them. I also think that the fix is technically not the best solution. The reason is that IMHO it is considered good practice for a object to always have all its attributes, else it is considered a different object or should make use of getters and setters, hence the removal of the conditional default object. This way the object is always the same regardless. Over time it should reduce mistakes and reduce code duplication because we not having to do extra checks every time we want to access a object variable.

Comment #5

kevinquillen CreditAttribution: kevinquillen commented 11 March 2013 at 13:39

Not to sidetrack, is there a reason we're adjusting code for PHP 5.4 compliance even though it isn't required to run Drupal or offered by a lot of hosts yet? I just want to double check that we won't introduce issues for folks on 5.3.x (or even 5.2 - though D7 sites should be 5.3.x).

Comment #6

NaX CreditAttribution: NaX commented 12 March 2013 at 07:14

@kevinquillen
According to the System requirements page the PHP requirements are:

Drupal 7: PHP 5.2.5 or higher (5.3 recommended).
Drupal 8: PHP 5.3.5 or higher.

Our host is currently migrating to PHP 5.4 and PHP 5.3 support will be dropped by the end of the year.

IMHO we should always try to support the latest version of PHP so to prevent major problems when upgrading PHP versions.
We hosts many Drupal sites and the PHP 5.3 upgrade was very painful I would like to avoid as much pain as possible with PHP 5.4.
From PHP 5.2 and 5.3 up it much easier to support the latest version of PHP without breaking compatibility with older versions.

Ref:
Lots of activity in core relating 5.4
#1498574: [policy, no patch] Require PHP 5.4

Just my 2cents :)

Comment #7

kevinquillen CreditAttribution: kevinquillen commented 12 March 2013 at 12:49

I understand that- just checking that there wasn't further effort like shortening code with array dereferencing and the like (definitely breaking < 5.4).