gn36 / phpbb-ext-versionchecknotifier Goto Github PK

The build script still references hookup in some parts.

This should be limited to administrators, who can manage extensions already?

PHP 5.3 and possibly other PHP Versions don't ship gmp functions, therefore the notifications can't be created.

Maybe replace by bcmath? Otherwise, another bignumber lib is necessary.

The Tests for sqlite3 time out and thereby cause phpunit to error. If run locally, the same error as here is encountered:

1. gn36\hookup\tests\functional\redirector_test::test_call_redirect
   Guzzle\Http\Exception\CurlException: [curl] 28: Operation timed out after 30032 milliseconds with 0 bytes received [url] app.php/versionchecknotifier/redirect/1?sid=ec

The same test being run by itself works fine in the same environment.

Potentially a cross testing effect between multiple functional tests? The test runs a whole 2.9 minutes by itself, while all tests together take only 3.17 minutes.

This error causes https://www.phpbb.com/customise/db/extension/versionchecknotifier/support/topic/167836?p=519861#p519861

The static function get_notification_id calculates the id incorrectly.

1. It uses ext_name instead of name
2. The version information provided by the cronjob is actually an array. The version information stored, however, is only a string. Depending on what is provided, the information differs and therefore the id is calculated differently (including a suppresed error notice of array to string conversion).

Multiple possible solutions:

1. Calculate ID using internal static function to find the correct new version (slow, seems to cause issues with unittests, yields same ID every call)
2. simply use a serialized version of the complete array for ID calculation (simple, but has the same problem of different ids)
3. Move version extraction to cronjob / helper class (should simplify id calculation in notification, yields the same id on every call)

If the same ID is given on every call this is correct behavior. But it changes the behavior of this extension since right now, administrators are reminded frequently about the same outdated extension. So this should be corrected, but maybe it would be best to remark the notification as unread or delete and reapply it. Maybe this can also be used as a basis for a user defined setting on how frequently he wishes to be notified.

The redirect could ask for a login if a non-logged in user tries to use it.

The constructor has changed and must be adjusted. Other changes from phpBB.com Notes:

Notifications (3.2 only)
If your extension uses the notifications system by extending the notifications class, then you should take a look at how the notifications classes have changed for 3.2. The notable changes are made to the find_users_for_notification and create_insert_array methods.

public function find_users_for_notification($data, $options = array())
{
    ...
    // phpBB 3.1 usage:
    $users[$row['user_id']] = array('');

   // phpBB 3.2 usage:
    $users[$row['user_id']] = $this->notification_manager->get_default_methods();
    ...
} 

public function create_insert_array($data, $pre_create_data = array())
{
    ...
    // phpBB 3.1 usage:
    return parent::create_insert_array($data, $pre_create_data);

   // phpBB 3.2 usage:
    parent::create_insert_array($data, $pre_create_data);
} 

Or installed/enabled

When redirecting to an extension download from a new notification, phpBB fails the redirect with an error.

The redirect may need to be carried out differently with an intermediate page inbetween.

Emails sent out by versionchecknotifier contain & instead of &. This causes malfunctioning urls.

The build script should include txt files, otherwise Emails won't work.

Instead of generating one notification per day/cronjob execution, a switch could be added to check for existing notifications and only inform about new updates.