Implement return types and type hints

[Sama34]

With the intention to standardize the MyAlerts plugin and get it ready for 1.9, I suggest the following changes:

• Implement type hints
• Implement return types (avoid union types for now)
• Update functions and methods arguments names for PHP8.4 named arguments
• Implement exception handling
• Implement PSR12 as coding style

Discuss and feedback welcome.

[lairdshaw]

It seems like a good idea to me, albeit that I'm not yet very familiar with PSR12. If @euantorano doesn't have any objections, then let's go with it.

A couple of issues I've discovered with the PR as it is are that:

1. When $config['database']['type'] == 'mysqli', then

   MyAlerts/inc/plugins/MybbStuff/MyAlerts/src/AlertTypeManager.php

   Line 71 in 2773d05

   $alertType->setId($row['id']);

   triggers a TypeError, because mysqli_fetch_array, which the ->fetch-array($query) above calls in turn, seems to return a string for every value in the array, no matter what column type it is in the database, i.e., including integer types. MybbStuff_MyAlerts_Entity_AlertType::setId() is, however, now expecting an integer (and is getting a string). I guess you didn't test with this configured database type. I've confirmed that when it's instead configured to be 'mysql_pdo', an integer is returned in the array as expected. Presumably the same is true for 'pgsql_pdo', and perhaps also for 'pgsql' and 'sqlite'.

2. MyAlerts/inc/plugins/MybbStuff/MyAlerts/src/Entity/AlertType.php

   Line 44 in 2773d05

   $alertType->setEnabled($serialized['enabled']);

   also triggers a TypeError, because $serialized['enabled'] is set to the integer 1, whereas MybbStuff_MyAlerts_Entity_AlertType::setEnabled() is now expecting a boolean.
3. The fixes of your second commit starting here don't resolve all errors, because on initial install, $this->mybb->user['myalerts_disabled_alert_types'] isn't set at all (the array key doesn't exist), so it's necessary to test for that scenario too:

   MyAlerts/inc/plugins/MybbStuff/MyAlerts/src/AlertManager.php

   Line 84 in 2773d05

   is_array($this->mybb->user['myalerts_disabled_alert_types']) ?

More if/as I discover other problems.

[lairdshaw]

Fixed the link in the first numbered item.

[Sama34]

Hi, yes, I plan this PR to fix issues over time as they are catch.

We should probably consider to not declare strict types, as to minimize backwards compatibility issues.

If so, I will remove the strict declaration till the end.

Regards.

[euantorano]

I'm all for type hinting wherever possible. Very glad to see a push for adding it throughout the plugin, especially as the currently supported PHP versions all support it.

I would argue that keeping the strict declaration in from the start makes it far easier to catch any issues with the type hints.

[lairdshaw]

I would argue that keeping the strict declaration in from the start makes it far easier to catch any issues with the type hints.

That does after all seem best. The sort of issues I noted in my first comment can then simply be fixed as they arise.