celtic-project / LTI-PHP

PHP class library for building LTI integrations
GNU Lesser General Public License v3.0
48 stars 38 forks source link

getSetting() strictly and prohibitively requires return type string #78

Closed etraininteractive closed 6 months ago

etraininteractive commented 8 months ago

Since the PHP 8 update, getSetting() strictly requires strings to be returned. This is counterintuitive for settings that should not have a strict type given their multipurpose application

For example, we were storing ints in the settings object prior to the update, yet attempting to retrieve these settings is causing the below error. setSetting() calls getSetting too, making updating the setting to a string problematic too.

image

https://github.com/celtic-project/LTI-PHP/blob/3173354fa34708f639d9dd518bc0e3168430a862/src/ResourceLink.php#L415 https://github.com/celtic-project/LTI-PHP/blob/3173354fa34708f639d9dd518bc0e3168430a862/src/System.php#L282

I'd suggest not using a strict type for settings, however at the very least allowing string|int|float|bool|array|object.

spvickers commented 8 months ago

The settings methods have always expected string values, and have long been documented as such. I would recommend that you convert values of other types into strings before setting them, and convert them back after retrieving them,.

etraininteractive commented 8 months ago

Ok, what is the benefit of this restriction? Consider this a feature request then.

spvickers commented 8 months ago

I guess the main benefit is that the datatype of the value returned by calls to getSetting will be known to be a string and so no code is required to check it. The use of string values is in line with the LTI spec. Pre LTI 1.3 all message parameters were passed as strings (just as the PHP superglobals $_GET and $_POST contain string values for the parameters received). Even with LTI 1.3 all custom parameters must be of type string. Keeping this property to strings also keeps it simpler for dataconnectors, especially bespoke ones which might use existing data structures for storing tool and/or platform data.

However, the workaround I suggested could be incorporated into new methods such as setIntSetting and getIntSetting which automatically perform the conversion of an integer to/from a string.