Mismatched function declaration, update_url() in lib/activity.php

Bug #1605127 reported by Howard Miller
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Mahara
Fix Released
Medium
Aaron Wells
15.04
Fix Released
Medium
Aaron Wells
15.10
Fix Released
Medium
Aaron Wells
16.04
Fix Released
Medium
Aaron Wells
16.10
Fix Released
Medium
Aaron Wells

Bug Description

Branch 16.04

In lib/activity.php at approx line 710 the function update_url() is declared. It has no parameters.

However, at approx line 750 it is called and a parameter ($userdata->internalid) is used. One of them must be wrong, surely?

Tags: php7
Revision history for this message
Aaron Wells (u-aaronw) wrote :

Hi Howard,

You are correct. The abstract "Activity" class defines update_url() with no parameters, but then calls it with one parameter a few lines later.

The excess parameter itself actually doesn't cause any problems. If you're calling $this->update_url($something) on an Activity subclass that doesn't override update_url(), then PHP will silently accept it and assume you're doing a variable-length argument list ( http://php.net/manual/en/functions.arguments.php#functions.variable-arg-list ). In PHP 5.5 it won't even print an E_STRICT warning or anything. In PHP 5.6 and up, it might, since they've added the "..." syntax to explicitly indicate a variable-length argument list.

The bigger problem here is the mismatch in the signature of the update_url() method between Activity and its subclasses. That will throw an E_STRICT warning in PHP 5, but it's a fatal error in PHP 7.

Revision history for this message
Mahara Bot (dev-mahara) wrote : A patch has been submitted for review

Patch for "master" branch: https://reviews.mahara.org/6744

Aaron Wells (u-aaronw)
tags: added: php7
Revision history for this message
Kristina Hoeppner (kris-hoeppner) wrote : Re: Mismatched function declaration, urdate_url() in lib/activity.php

Howe would I test this patch on the front-end, please?

Revision history for this message
Howard Miller (howardsmiller) wrote :

We saw it upgrading (command line with debugging up full) from 15.04 to 16.04. Loads of warnings towards the start of the upgrade.

Revision history for this message
Kristina Hoeppner (kris-hoeppner) wrote :

Howard, would it be possible for you then to test and verify the patch?

Revision history for this message
Howard Miller (howardsmiller) wrote :

Ye-es but it might take me a few days to get to.

Revision history for this message
Kristina Hoeppner (kris-hoeppner) wrote :

Fantastic, Howard!

Revision history for this message
Aaron Wells (u-aaronw) wrote :

On further inspection, it looks like having an mismatched function declaration in a child class results in an E_WARNING message in PHP7, not a fatal error. (See http://php.net/manual/en/migration70.incompatible.php#migration70.incompatible.error-handling.strict )

And although that page seems to imply that it would generate an E_STRICT message in PHP5, I'm not seeing any such E_STRICT in my testing, no matter what I set my error level to.

Revision history for this message
Aaron Wells (u-aaronw) wrote :

Hi Kristina & Howard,

You can test out that patch by doing this:

1. Send a friend request (so that ActivityType::update_url() gets run)
2. Send a user a private message (so that ActivityTypeMultirecipientnotification::update_url() gets run)

Just test that both notifications go through successfully, that any links in the message or the emails are correct, and that there aren't any error messages in the logs.

Cheers,
Aaron

Aaron Wells (u-aaronw)
summary: - Mismatched function declaration, urdate_url() in lib/activity.php
+ Mismatched function declaration, update_url() in lib/activity.php
Revision history for this message
Mahara Bot (dev-mahara) wrote : A change has been merged

Reviewed: https://reviews.mahara.org/6744
Committed: https://git.mahara.org/mahara/mahara/commit/b469030b12e80147d406fc00b75afe024a329402
Submitter: Robert Lyon (<email address hidden>)
Branch: master

commit b469030b12e80147d406fc00b75afe024a329402
Author: Aaron Wells <email address hidden>
Date: Mon Jul 25 10:56:35 2016 +1200

Bug 1605127: Method signature mismatch in Activity::update_url()

The abstract Activity class defines the function update_url()
with no parameters, but the two subclasses that override it
define it with one parameter. This will cause problems in PHP 7.

It's always called with one parameter, (even in the Activity
class itself) so the best option is to add one parameter to the
implementation in Activity.

Change-Id: I810061ed6f8c55101327e2e907bb68ebf9870380
behatnotneeded: Covered by existing tests

Revision history for this message
Mahara Bot (dev-mahara) wrote : A patch has been submitted for review

Patch for "16.04_STABLE" branch: https://reviews.mahara.org/6768

Revision history for this message
Mahara Bot (dev-mahara) wrote :

Patch for "15.10_STABLE" branch: https://reviews.mahara.org/6769

Revision history for this message
Mahara Bot (dev-mahara) wrote :

Patch for "15.04_STABLE" branch: https://reviews.mahara.org/6770

Revision history for this message
Mahara Bot (dev-mahara) wrote : A change has been merged

Reviewed: https://reviews.mahara.org/6769
Committed: https://git.mahara.org/mahara/mahara/commit/bd05e5b5a27999cb37569115d5460ba132df7d65
Submitter: Robert Lyon (<email address hidden>)
Branch: 15.10_STABLE

commit bd05e5b5a27999cb37569115d5460ba132df7d65
Author: Aaron Wells <email address hidden>
Date: Mon Jul 25 10:56:35 2016 +1200

Bug 1605127: Method signature mismatch in Activity::update_url()

The abstract Activity class defines the function update_url()
with no parameters, but the two subclasses that override it
define it with one parameter. This will cause problems in PHP 7.

It's always called with one parameter, (even in the Activity
class itself) so the best option is to add one parameter to the
implementation in Activity.

Change-Id: I810061ed6f8c55101327e2e907bb68ebf9870380
behatnotneeded: Covered by existing tests
(cherry picked from commit b469030b12e80147d406fc00b75afe024a329402)

Revision history for this message
Mahara Bot (dev-mahara) wrote :

Reviewed: https://reviews.mahara.org/6768
Committed: https://git.mahara.org/mahara/mahara/commit/01ad202fd4f06999f9831635c14c700d465d0f60
Submitter: Robert Lyon (<email address hidden>)
Branch: 16.04_STABLE

commit 01ad202fd4f06999f9831635c14c700d465d0f60
Author: Aaron Wells <email address hidden>
Date: Mon Jul 25 10:56:35 2016 +1200

Bug 1605127: Method signature mismatch in Activity::update_url()

The abstract Activity class defines the function update_url()
with no parameters, but the two subclasses that override it
define it with one parameter. This will cause problems in PHP 7.

It's always called with one parameter, (even in the Activity
class itself) so the best option is to add one parameter to the
implementation in Activity.

Change-Id: I810061ed6f8c55101327e2e907bb68ebf9870380
behatnotneeded: Covered by existing tests
(cherry picked from commit b469030b12e80147d406fc00b75afe024a329402)

Revision history for this message
Mahara Bot (dev-mahara) wrote :

Reviewed: https://reviews.mahara.org/6770
Committed: https://git.mahara.org/mahara/mahara/commit/3b1580d9f1e6d055e6b9d530900cd09f06745b29
Submitter: Aaron Wells (<email address hidden>)
Branch: 15.04_STABLE

commit 3b1580d9f1e6d055e6b9d530900cd09f06745b29
Author: Aaron Wells <email address hidden>
Date: Mon Jul 25 10:56:35 2016 +1200

Bug 1605127: Method signature mismatch in Activity::update_url()

The abstract Activity class defines the function update_url()
with no parameters, but the two subclasses that override it
define it with one parameter. This will cause problems in PHP 7.

It's always called with one parameter, (even in the Activity
class itself) so the best option is to add one parameter to the
implementation in Activity.

Change-Id: I810061ed6f8c55101327e2e907bb68ebf9870380
behatnotneeded: Covered by existing tests
(cherry picked from commit b469030b12e80147d406fc00b75afe024a329402)

Robert Lyon (robertl-9)
Changed in mahara:
status: Fix Committed → Fix Released
milestone: 16.10.0 → none
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.