Problem/Motivation

This module is mostly missing types for parameters in function declarations. Recent issues have change function parameters on SubscriberInterface and SubscriptionManagerInterface, so let's start by adding types there to help people realise to change their code.

This will also catch some places inside the module that don't follow the interface properly. For example, the second argument to Subscriber::loadByMail and Subscriber::loadByUid is frequently passed as 'create' not TRUE. But the interface specifies this should be a boolean. It doesn't make any difference to the result (as the implementation currently stands), but could cause unexpected bugs if the implementation is changed.

CommentFileSizeAuthor
#8 simplenews.param-types.3260400-interdiff-6-8.txt5.44 KBadamps
#8 simplenews.param-types.3260400-8.patch11.62 KBadamps
#6 simplenews.param-types.3260400-6.patch7.71 KBadamps

Comments

jonathanshaw created an issue. See original summary.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Issue summary: View changes
adamps’s picture

adamps commented
Version: 3.x-dev » 4.x-dev

I like passing 'create' because it's much more informative to the reader than just TRUE.

What we can do is to add types to the function arguments. This would then force conversion to bool.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

> What we can do is to add types to the function arguments. This would then force conversion to bool.

That's now how types work. Types don't convert automatically, that will give you a fatal error.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented

I like passing 'create' because it's much more informative to the reader than just TRUE.

So let's used string or mixed in the interface, rather than violating our own interface. Someone swapping out the Subscriber class could get bitten if they believed what the interface said.

adamps’s picture

adamps commented
Status: Active » Needs review
StatusFileSize
new simplenews.param-types.3260400-6.patch7.71 KB

I realise it's a little strange that I pass strings like this for bool - I'm content to give it up. Here's a patch that adds types to SubscriberInterface - let's see what happens.

Status: Needs review » Needs work

The last submitted patch, 6: simplenews.param-types.3260400-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

adamps’s picture

adamps commented
Status: Needs work » Needs review
StatusFileSize
new simplenews.param-types.3260400-8.patch11.62 KB
new simplenews.param-types.3260400-interdiff-6-8.txt5.44 KB
adamps’s picture

adamps commented
Title: Pass TRUE not 'create' when loading subscriber » Add types to function parameters
Issue summary: View changes

That's now how types work. Types don't convert automatically, that will give you a fatal error.

It seems there are some specific exceptions where types do convert automatically. See https://www.php.net/manual/en/language.types.type-juggling.php#language.....

In this context the value must be a value of the type. Two exceptions exist, the first one is: if the value is of type int and the declared type is float, then the integer is converted to a floating point number. The second one is: if the declared type is a scalar type, the value is convertable to a scalar type, and the coercive typing mode is active (the default), the value may be converted to an accepted scalar value. See below for a description of this behaviour.

adamps’s picture

adamps commented
adamps’s picture

adamps commented
Status: Needs review » Fixed

  • AdamPS committed 0560fcf4 on 4.x 
    Issue #3260400 by AdamPS: Add types to function parameters

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Follow-up: #3402475: Subscriber::loadByMail(): Argument #1 ($mail) must be of type string, null given