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

[Howard Miller]

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?

[Aaron Wells (u-aaronw)]

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.

[Mahara Bot (dev-mahara)]

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

[Kristina Hoeppner (kris-hoeppner)]

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

[Howard Miller (howardsmiller)]

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.

[Kristina Hoeppner (kris-hoeppner)]

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

[Howard Miller (howardsmiller)]

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

[Kristina Hoeppner (kris-hoeppner)]

Fantastic, Howard!

[Aaron Wells (u-aaronw)]

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.

[Aaron Wells (u-aaronw)]

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

[Mahara Bot (dev-mahara)]

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

[Mahara Bot (dev-mahara)]

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

[Mahara Bot (dev-mahara)]

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

[Mahara Bot (dev-mahara)]

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

[Mahara Bot (dev-mahara)]

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)

[Mahara Bot (dev-mahara)]

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)

[Mahara Bot (dev-mahara)]

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)