Enh: Code de-duplication by introducing PermissionManager::handlePermissionStateChange()

[martin-rueegg]

Code de-duplication by introducing PermissionManager::handlePermissionStateChange()

PR Admin

What kind of change does this PR introduce?

• Refactor

Does this PR introduce a breaking change?

• No

The PR fulfills these requirements

• It's submitted to the develop branch, not the master branch if no hotfix
• When resolving a specific issue, it's referenced in the PR's description (e.g. Fix #xxx[,#xxx], where "xxx" is the Github issue number)
• All tests tests are passing
• New/updated tests are included
• Changelog was modified

If adding a new feature, the PR's description includes:

• A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

[martin-rueegg]

@luke- I'm quite puzzled at the failing tests again:

https://github.com/humhub/humhub/actions/runs/7232996921/job/19707789671#step:18:1557 seem to be complete bullocks, as the string <time class="tt time timeago" datetime="2023-12-16T12:00:00+00:00" title="Dec 16, 2023 - 12:00 PM">Dec 16, 2023 - 12:00 PM</time> does actually contain 12:00 PM</time>, as far as I can tell! :-)

The other error in ProfileCest should be fixed with the new push.

[martin-rueegg]

@luke- I'm quite puzzled at the failing tests again:

https://github.com/humhub/humhub/actions/runs/7232996921/job/19707789671#step:18:1557 seem to be complete bullocks, as the string <time class="tt time timeago" datetime="2023-12-16T12:00:00+00:00" title="Dec 16, 2023 - 12:00 PM">Dec 16, 2023 - 12:00 PM</time> does actually contain 12:00 PM</time>, as far as I can tell! :-)

Locally, the tests pass.

[martin-rueegg]

@martin-rueegg Why do we need the methods `checkBool` and `checkFloat` if they are not used?

We don't need them. I added them for reasons of consistency. If you want, I can certainly replace some code in other parts of the Core, but I wanted to keep the PR focused.

[luke-]

What do you think about introducing a new helper class humhub\helpers\DatatypeHelper and put the methods there?

I assume the method names are aligned with the existing checkClassType, but maybe filterString would be a better naming here?

Is it useful that e.g. filterString can also return null? Then an additional null check would still be necessary here. Just as a thought. We can also leave it as it is.

[martin-rueegg]

What do you think about introducing a new helper class humhub\helpers\DatatypeHelper and put the methods there?

Sounds good to me.

I assume the method names are aligned with the existing checkClassType, but maybe filterString would be a better naming here?

Correct.

If we put the filterString in an humhub\helpers\DatatypeHelper, I'd suggest to put checkClassType there, too.

But maybe we should do a seperate PR for that move?

Is it useful that e.g. filterString can also return null? Then an additional null check would still be necessary here. Just as a thought. We can also leave it as it is.

Well, I don't really see an additional value in returning a boolean to tell me, if a variable is a string or not:

if (checkString($string)) vs if (checkString($string) !== null) is about the same. However, returning ?string fives me additional usage possibilites, like "return filterInt($intOrString) ?? filterString($intOrString) ?? nullwhich makes sure the return value is anintif it can be converted, astringornull` otherwise..

filter* sounds good to me as well.

Shall we

• use humhub\helpers\DatatypeHelper class?
• use filter* naming convention
• also move checkClassType
• also rename checkClassType to filterClassType (it follows the same paradigm of returning null if invalid).
• do the checkClassType move/rename in this PR?

[luke-]

@martin-rueegg Thank you sounds good to me.

We can mark checkClassType as deprecated and pass it to the new implementation.

[martin-rueegg]

@martin-rueegg Thank you sounds good to me.

We can mark checkClassType as deprecated and pass it to the new implementation.

Ok cool. Just let me know what of the above checklist do you want in this PR? All of it? Or just check the ones you prefer.

[martin-rueegg]

I've now done all in this one PR.

Also, I've added tests for the new methods.

[luke-]

@martin-rueegg Thank you for the implementation. From my side, we can add the DataTypeHelper as you implemented.

Although, you know my philosophy, I always prefer implementations according to the KISS principle.

Ideally something like this, which can be understood quite easy:

    public static function filterFloat(mixed $value, bool $strict = false, bool $throwErrorOnStrict = false): ?float
    {
        if (!is_float($value) && $strict) {
            if ($throwErrorOnStrict) {
                throw new InvalidArgumentException('No float given!');
            }
            return null;
        }

        return floatval($value);
    }

@yurabakhtin Can you please also take a look into this?

[yurabakhtin]

Ideally something like this, which can be understood quite easy:

    public static function filterFloat(mixed $value, bool $strict = false, bool $throwErrorOnStrict = false): ?float
    {
        if (!is_float($value) && $strict) {
            if ($throwErrorOnStrict) {
                throw new InvalidArgumentException('No float given!');
            }
            return null;
        }

        return floatval($value);
    }

@yurabakhtin Can you please also take a look into this?

@luke- Yes, your suggested method code looks simpler, maybe we could use the simple fucntion floatvar instead of filter_var.

[martin-rueegg]

I'll have a look again. Not aware of floatval. But I thing throwing an exception should also be possible if not strict...

[luke-]

@martin-rueegg Here are a few more remarks. I would also like to limit the flexibility a bit (make checkType() private). Besides that, we can merge the PR from my side.

[luke-]

I would prefer the method to be private.

Suggested change

	public static function checkType($input, $types, ?bool $requireAll = false, ?string $throw = '$input', ?array &$typesChecked = null): ?string
	private static function checkType($input, $types, ?bool $requireAll = false, ?string $throw = '$input', ?array &$typesChecked = null): ?string

[martin-rueegg]

I think that would be a real shame. This function is complementing the filterClassType() for types that are not necessarily class names or instances.

Particularly, since many functions return one data type on success (eg. an array of records) or false on error (rather than null), this method makes it very easy to check such an input parameter:

/**
 * @param array|false $result from previous function
 */
public function foo($result) { // <- no strict type possible here!
    DataTypeHelper::checkType($result, ['array', 'bool']);

   // proceed with code
}

So I personally would see filterInt($var) as a shortcut to checkType($var, 'int')

Can we keep this public?

[luke-]

I understand that the method is very powerful, but at the same time it is also very complex.

For example, when I see the following call somewhere in the code, I am completely confused:

$someString = DataTypeHelper::checkType($value, ['array', 'string'], false, '$value', &$typesChecked);

I would be completely fine with a method like, which throws an exception on failure

DataTypeHelper::ensureType(mixed $value, array $types) : void;

A few other thoughts/ambiguities for checkType() which is why I want to keep it private:

• Why ?string as return type? I would expect bool for such type of helper
• Do we really need $requireAll option? I would drop it for simplicity.
• I understand the $throw is useful in some situations, but I don't like the style to pass a variable name as string.
• I roughly remember that you explained the purpose of &$typesChecked
• Before we introduce a more complex type validation/helper API (500 lines+ ), we should perhaps also check whether there are ready-made libraries for this. We can very easily replace helper methods filterInt() with other implementations later on. However, this will not be possible with more complex methods such as checkType().

[martin-rueegg]

I would be completely fine with a method like, which throws an exception on failure

DataTypeHelper::ensureType(mixed $value, array $types) : void;

How about this:

• One "ensure" method (throwing an exception)

  DataTypeHelper::ensureType($input, array $types, string $parameterOrMessage = '$input'): void

• One "check" method (returning the type)

  DataTypeHelper::checkType($input, array $types, bool $returnIndex = false): ?string

.

Why ?string as return type? I would expect bool for such type of helper

Because like that you can use it e.g. in a switch statement like so:

switch (DataTypeHelper::checkType($value, ['string', 'int', Stringable::class])) {
    case Stringable::class:
        $value = (string)$value;
    case 'string':
        // Do something with a string value
        break;
        
    case 'int':
        // do something else with an int
        break;
        
    default:
        // do nothing
}

.

Do we really need $requireAll option? I would drop it for simplicity.

Nope. Could be handy if you want to test two interfaces at once. But in such a case just use the method twice ...

.

I understand the $throw is useful in some situations, but I don't like the style to pass a variable name as string.

Better with the above suggestion?

.

I roughly remember that you explained the purpose of &$typesChecked

I've removed it.

[luke-]

I would be completely fine with a method like, which throws an exception on failure

DataTypeHelper::ensureType(mixed $value, array $types) : void;

How about this:

• One "ensure" method (throwing an exception)

  DataTypeHelper::ensureType($input, array $types, string $parameterOrMessage = '$input'): void
  

• One "check" method (returning the type)

  DataTypeHelper::checkType($input, array $types, bool $returnIndex = false): ?string
  

Why ?string as return type? I would expect bool for such type of helper

Because like that you can use it e.g. in a switch statement like so:

switch (DataTypeHelper::checkType($value, ['string', 'int', Stringable::class])) {
    case Stringable::class:
        $value = (string)$value;
    case 'string':
        // Do something with a string value
        break;
        
    case 'int':
        // do something else with an int
        break;
        
    default:
        // do nothing
}

Ok, I see. How about then calling the method DataTypeHelper::getType(mixed $mixed, array $allowedTypes): ?string?

So:

• DataTypeHelper::ensureType(mixed $input, array $allowedTypes): void
• DataTypeHelper::getType(mixed $input, array $allowedTypes): ?string
• DataTypeHelper::hasType(mixed $input, array $allowedTypes): bool

---

I'm still not sure, about:

• Passing the variable name as string to the ensureType() method. For me the code usage looks cluttered. if (DataTypeHelper::ensureType($param1, ['string'], '$param1') && DataTypeHelper::ensureType($param2, ['int'], '$param2')) { /* ... */ }
• I'm thinking about raising the HH minVersion to 8.1 and use Enums instead of strings

[martin-rueegg]

Ok, I see. How about then calling the method DataTypeHelper::getType(mixed $mixed, array $allowedTypes): ?string?

So:

• DataTypeHelper::ensureType(mixed $input, array $allowedTypes): void
• DataTypeHelper::getType(mixed $input, array $allowedTypes): ?string
• DataTypeHelper::hasType(mixed $input, array $allowedTypes): bool

Sure, that could be done.

However, hasType is kinda just an alias to getType: except for the strict type, the later would always yield the same result as the former, since only the empty string "" would yield false, but that could never happen. In such a case null would be returned which is == to false:

if (DataTypeHelper::getType("some string", ['string', 'int'])) {
    // result "string" is "true"
}

The only time you would actually need to use DataTypeHelper::hasType() (with a strict boolean return type) is when you'd pass the result straight to another fixed-type method:

function foo(bool $isString) {
   // $isString cannot be auto-converted here.
}

// so this would fail:
foo(DataTypeHelper::getType("some string", ['string', 'int'])));

// instead, this would be required:
DataTypeHelper::getType("some string", ['string', 'int']) !== null); 

As such, that would also be the exact implementation:

public function hasType($input, array $allowedTypes): bool {
    return DataTypeHelper::getType($input, $allowedTypes) !== null);
}

I'm still not sure, about:

• Passing the variable name as string to the ensureType() method. For me the code usage looks cluttered. if (DataTypeHelper::ensureType($param1, ['string'], '$param1') && DataTypeHelper::ensureType($param2, ['int'], '$param2')) { /* ... */ }

I understand. Well, in such a case, where you only have one possible type, you could use a strictly-typed parameter, which is always the better option anyway. (But I don't think that was the point of your argument.)

Furthermore you would no longer need the if. And rewriting it without the && would certainly increase readability:

function bar($param1, $param2) {
    DataTypeHelper::ensureType($param1, ['string', 'int'], '$param1');
    DataTypeHelper::ensureType($param2, [someClass::class, 'is_scalar'], 'Please make sure $param2 has the right format');

    /* continue with type-safe values here */
}

Besides, the parameter is optional. But it helps to write a meaningful exception message. Which also is as close to the PHP's own type exception (naming the parameter name and number).

If you do not want to throw an exception, but just do something if a given type is povided, then you do no longer have that parameter:

if (DataTypeHelper::hasType($param1, ['string', 'int']) && DataTypeHelper::hasType($param2, [someClass::class, 'is_scalar']) {
     /* do something conditionally with type-safe values here */
}

• I'm thinking about raising the HH minVersion to 8.1 and use Enums instead of strings

You mean to raise the PHP min version?

I'm not quite sure what would enum's help in this context? For the $allowedTypes parameter?

[luke-]

Ok, I see. How about then calling the method DataTypeHelper::getType(mixed $mixed, array $allowedTypes): ?string?
So:

• DataTypeHelper::ensureType(mixed $input, array $allowedTypes): void
• DataTypeHelper::getType(mixed $input, array $allowedTypes): ?string
• DataTypeHelper::hasType(mixed $input, array $allowedTypes): bool

Sure, that could be done.

However, hasType is kinda just an alias to getType: except for the strict type, the later would always yield the same result as the former, since only the empty string "" would yield false, but that could never happen. In such a case null would be returned which is == to false:

if (DataTypeHelper::getType("some string", ['string', 'int'])) {
    // result "string" is "true"
}

The only time you would actually need to use DataTypeHelper::hasType() (with a strict boolean return type) is when you'd pass the result straight to another fixed-type method:

function foo(bool $isString) {
   // $isString cannot be auto-converted here.
}

// so this would fail:
foo(DataTypeHelper::getType("some string", ['string', 'int'])));

// instead, this would be required:
DataTypeHelper::getType("some string", ['string', 'int']) !== null); 

As such, that would also be the exact implementation:

public function hasType($input, array $allowedTypes): bool {
    return DataTypeHelper::getType($input, $allowedTypes) !== null);
}

I'm still not sure, about:

• Passing the variable name as string to the ensureType() method. For me the code usage looks cluttered. if (DataTypeHelper::ensureType($param1, ['string'], '$param1') && DataTypeHelper::ensureType($param2, ['int'], '$param2')) { /* ... */ }

I understand. Well, in such a case, where you only have one possible type, you could use a strictly-typed parameter, which is always the better option anyway. (But I don't think that was the point of your argument.)

Furthermore you would no longer need the if. And rewriting it without the && would certainly increase readability:

function bar($param1, $param2) {
    DataTypeHelper::ensureType($param1, ['string', 'int'], '$param1');
    DataTypeHelper::ensureType($param2, [someClass::class, 'is_scalar'], 'Please make sure $param2 has the right format');

    /* continue with type-safe values here */
}

Besides, the parameter is optional. But it helps to write a meaningful exception message. Which also is as close to the PHP's own type exception (naming the parameter name and number).

If you do not want to throw an exception, but just do something if a given type is povided, then you do no longer have that parameter:

if (DataTypeHelper::hasType($param1, ['string', 'int']) && DataTypeHelper::hasType($param2, [someClass::class, 'is_scalar']) {
     /* do something conditionally with type-safe values here */
}

• I'm thinking about raising the HH minVersion to 8.1 and use Enums instead of strings

You mean to raise the PHP min version?

I'm not quite sure what would enum's help in this context? For the $allowedTypes parameter?

For me, I would prefer instead of if (static::getType($var, ['string', 'int']) === 'string') { ... } something like: if (static::getType($var, [DataType::String, DataType::Integer]) === DataType::String) { ... }.

[martin-rueegg]

• I'm thinking about raising the HH minVersion to 8.1 and use Enums instead of strings

You mean to raise the PHP min version?
I'm not quite sure what would enum's help in this context? For the $allowedTypes parameter?

For me, I would prefer instead of if (static::getType($var, ['string', 'int']) === 'string') { ... } something like: if (static::getType($var, [DataType::String, DataType::Integer]) === DataType::String) { ... }.

I see.

Well, if you raise PHP to v8.1, then I'm happy to create such an Enum.

Otherwise, I'm happy to create regular constants in the current DataTypeHelper class.

Please note, that validation of the input types can not be done exclusively by enum, as class, interface and trait names are variable.

[martin-rueegg]

Thanks @luke- for your review and comments.

[martin-rueegg]

I think that would be a real shame. This function is complementing the filterClassType() for types that are not necessarily class names or instances.

Particularly, since many functions return one data type on success (eg. an array of records) or false on error (rather than null), this method makes it very easy to check such an input parameter:

/**
 * @param array|false $result from previous function
 */
public function foo($result) { // <- no strict type possible here!
    DataTypeHelper::checkType($result, ['array', 'bool']);

   // proceed with code
}

So I personally would see filterInt($var) as a shortcut to checkType($var, 'int')

Can we keep this public?

[martin-rueegg]

To have a consistent API, I have now streamlined the methods of DataTypeHelper as follows:

• Methods returning the input value if $value matches the given type(s), null or Exception otherwise:

  filterBool(      $value, ?bool $strict = false, bool $throwException = false): ?bool;
  filterFloat(     $value, bool $strict = false,  bool $throwException = false): ?float;
  filterInt(       $value, bool $strict = false,  bool $throwException = false): ?int;
  filterScalar(    $value, bool $strict = false,  bool $throwException = false);
  filterString(    $value, bool $strict = false,  bool $throwException = false): ?string;
  
  filterClassType( $value, $allowedTypes,         bool $throwException = false);
  filterType(      $value, $allowedTypes,         bool $throwException = false);

• Methods returning the the type name if it matches the given type, null or Exception otherwise:

  matchClassType($value, $allowedTypes, bool $throwException = false): ?string;
  matchType(     $value, $allowedTypes, bool $throwException = false, bool $returnIndex = false): ?string;

Additionally, there are some short-cuts:

• Methods returning the the type name if $value matches the given type(s), or throws an Exception otherwise:

  ensureClassType($value, $allowedTypes): string;
  ensureType(     $value, $allowedTypes): string;

• Methods returning boolean whether $value matches the given type(s):

  isClassType($value, $allowedTypes): bool;
  isType(     $value, $allowedTypes): bool;

[luke-]

@martin-rueegg Thanks for the overview.

The following questions:

• Could it be useful to split the class into two helpers? DataTypeHelper and ClassTypeHelper, since there are separate methods anyway? The class is already very large (/e.g. Class Type constants).

• Would not getType() be a better name instead of matchType(), then it is obvious what the method does.

  • I don't know if possible, but it could be cool if the method returns the data type if allowedTypes is null.
  • I'm sure there are some elegant use cases for $returnIndex. But for me this makes the method difficult to understand.

• Right now I am unsure about filterType(). For me, the filter* methods are there to safely migrate a value to a fixed defined data type. If I now allow multiple data types. It's not longer clear for me what happens in all case.

[martin-rueegg]

Could it be useful to split the class into two helpers? DataTypeHelper and ClassTypeHelper, since there are separate methods anyway? The class is already very large (/e.g. Class Type constants).

It is certainly possible. I don't consider it useful:

• They both use common code. E.g. the parseTypes() method, which would then have to become public and doesn't really belong to one class more than to the other. Same applies to the constants (which are already public, though).
• I'm not sure by what measure you consider a class big or small (kilobytes, number of lines, number of symbols) and what a good offset would be for you and for what reasons. In my perception it's quite a small class in all of the above dimensions. Tearing them apart would increase complexity (e.g. to use auto-completion).
• The main differences between eg. matchClassType and matchType are
  • matchClassType treats string $values as class names, not as arbitrary strings, similar to is_a().
  • Hence, only null, class names, interface names, and trait names make sense in the $allowedTypes parameter (where other data types and callables cause an exception)

Would not getType() be a better name instead of matchType(), then it is obvious what the method does.

Since that was your previous suggestion, I had considered that. However, I found it confusing, as the method does not necessarily return the type of the $value as get_debug_type() would. It does return the value of the type it matched within the $allowedTypes list. So for example, if you test an instance of a class that implements a trait or interface, and you have that trait or interface in the list of $allowedTypes, it will return the trait or interface name. As such, I find getType confusing.

We could of course add such a variant, which will behave more like get_debug_type by returning the real type, but does only so if any one type in $allowedTypes is matched. But then it might be better named e.g. getTypeIf($value, $allowedTypes)?

I don't know if possible, but it could be cool if the method returns the data type if allowedTypes is null.

That would be the same as get_debug_type(). So this would only make sense if the $allowedTypes is set dynamically. But a simple tenary would be more readable in this case, I guess:

$type = $allowedTypes === null ? get_debug_type($value) : DataTypeHelper::matchType($value, $allowedTypes);

I'm sure there are some elegant use cases for $returnIndex. But for me this makes the method difficult to understand.

I find it fascinating, that you struggle with an well-documented optional parameter. AFAIK you are using PhpStorm, too. There, all you need to do is to hoover the mouse over the method and it will pop-up the documentation to the function where the parameter is explained. Also, the parameter names are shown in front of the parameter. Does that not help clarifying? Also, I guess if the result is used in a real-life example, it would also be more explanatory:

$index = DataTypeHelper::matchType($row, ['array', ActiveRecord::class], false, true);

switch ($index ?? -1) {

case 0: // record is in array-format

    // do something with array
    break;

case 1: // record is an ActiveRecord instance

    // do something with record
    break;

default:
    // abort
   return;
}

Otherwise, would it be better to use a $flag parameter and some named numeric constants?

// definition
const THROW_EXCEPTION = 1;
const RETURN_INDEX = 2;
function DataTypeHelper::matchType($value, $allowedTypes, int $flags);

// usage
$index = DataTypeHelper::matchType($value, ['array', ActiveRecord::class], self::THROW_EXCEPTION + self::RETURN_INDEX);

That would make the calling code more expressive, once written. However, it would be more difficult to produce the code and would additionally require evaluation and validation of the flags.

Right now I am unsure about filterType(). For me, the filter* methods are there to safely migrate a value to a fixed defined data type. If I now allow multiple data types. It's not longer clear for me what happens in all case.

Fair enough. It can be removed.

[luke-]

Could it be useful to split the class into two helpers? DataTypeHelper and ClassTypeHelper, since there are separate methods anyway? The class is already very large (/e.g. Class Type constants).

It is certainly possible. I don't consider it useful:

• They both use common code. E.g. the parseTypes() method, which would then have to become public and doesn't really belong to one class more than to the other. Same applies to the constants (which are already public, though).

• I'm not sure by what measure you consider a class big or small (kilobytes, number of lines, number of symbols) and what a good offset would be for you and for what reasons. In my perception it's quite a small class in all of the above dimensions. Tearing them apart would increase complexity (e.g. to use auto-completion).

• The main differences between eg. matchClassType and matchType are

  • matchClassType treats string $values as class names, not as arbitrary strings, similar to is_a().
  • Hence, only null, class names, interface names, and trait names make sense in the $allowedTypes parameter (where other data types and callables cause an exception)

Ok, that was just an idea. Then let's leave it as it is.

Would not getType() be a better name instead of matchType(), then it is obvious what the method does.

Since that was your previous suggestion, I had considered that. However, I found it confusing, as the method does not necessarily return the type of the $value as get_debug_type() would. It does return the value of the type it matched within the $allowedTypes list. So for example, if you test an instance of a class that implements a trait or interface, and you have that trait or interface in the list of $allowedTypes, it will return the trait or interface name. As such, I find getType confusing.

Ok, understand. Then lets stay with matchType() as it is.

I'm sure there are some elegant use cases for $returnIndex. But for me this makes the method difficult to understand.

I find it fascinating, that you struggle with an well-documented optional parameter. AFAIK you are using PhpStorm, too. There, all you need to do is to hoover the mouse over the method and it will pop-up the documentation to the function where the parameter is explained. Also, the parameter names are shown in front of the parameter. Does that not help clarifying? Also, I guess if the result is used in a real-life example, it would also be more explanatory:

$index = DataTypeHelper::matchType($row, ['array', ActiveRecord::class], false, true);

switch ($index ?? -1) {

case 0: // record is in array-format

    // do something with array
    break;

case 1: // record is an ActiveRecord instance

    // do something with record
    break;

default:
    // abort
   return;
}

I don't want to have to rely on any helpers or comments to understand the code quickly. The code should be always simple as possible, even when a bit slower.

switch(DataTypeHelper::matchType($row, ['array', ActiveRecord::class]))
{
    case 'array':
         // do something
        break;

    case ActiveRecord::class:
         // do something
        break;

    default:
         // abort
        break;
}

Otherwise, would it be better to use a $flag parameter and some named numeric constants?

// definition
const THROW_EXCEPTION = 1;
const RETURN_INDEX = 2;
function DataTypeHelper::matchType($value, $allowedTypes, int $flags);

// usage
$index = DataTypeHelper::matchType($value, ['array', ActiveRecord::class], self::THROW_EXCEPTION + self::RETURN_INDEX);

That would make the calling code more expressive, once written. However, it would be more difficult to produce the code and would additionally require evaluation and validation of the flags.

Thanks for the suggestion. But I am less concerned with the parameter, which is reasonably clear, and more with how it is used.

Right now I am unsure about filterType(). For me, the filter* methods are there to safely migrate a value to a fixed defined data type. If I now allow multiple data types. It's not longer clear for me what happens in all case.

Fair enough. It can be removed.

[martin-rueegg]

I'm sure there are some elegant use cases for $returnIndex. But for me this makes the method difficult to understand.

I find it fascinating, that you struggle with an well-documented optional parameter. AFAIK you are using PhpStorm, too. There, all you need to do is to hoover the mouse over the method and it will pop-up the documentation to the function where the parameter is explained. Also, the parameter names are shown in front of the parameter. Does that not help clarifying? Also, I guess if the result is used in a real-life example, it would also be more explanatory:

$index = DataTypeHelper::matchType($row, ['array', ActiveRecord::class], false, true);

switch ($index ?? -1) {

case 0: // record is in array-format

    // do something with array
    break;

case 1: // record is an ActiveRecord instance

    // do something with record
    break;

default:
    // abort
   return;
}

I don't want to have to rely on any helpers or comments to understand the code quickly. The code should be always simple as possible, even when a bit slower.

switch(DataTypeHelper::matchType($row, ['array', ActiveRecord::class]))
{
    case 'array':
         // do something
        break;

    case ActiveRecord::class:
         // do something
        break;

    default:
         // abort
        break;
}

Sure, that's not the issue. I made a bad example. The issue is only when you use callables. Because you can't compare them as easily. But I'm sure another work-around can be found in such a case.

I'll drop the parameter and will make a new PR in case I have a strong use case.

[luke-]

Sure, that's not the issue. I made a bad example. The issue is only when you use callables. Because you can't compare them as easily. But I'm sure another work-around can be found in such a case.

I'll drop the parameter and will make a new PR in case I have a strong use case.

Thank you. Sorry if I'm a bit too specific. But especially with Helpers, better to be as compact as possible...

[martin-rueegg]

To have a consistent API, I have now streamlined the methods of DataTypeHelper as follows:

• Methods returning the input value if $value matches the given type(s), null or Exception otherwise:

  filterBool(  $value, ?bool $strict = false, bool $throwException = false): ?bool;
  filterFloat( $value,  bool $strict = false, bool $throwException = false): ?float;
  filterInt(   $value,  bool $strict = false, bool $throwException = false): ?int;
  filterScalar($value,  bool $strict = false, bool $throwException = false); // scalar
  filterString($value,  bool $strict = false, bool $throwException = false): ?string;

• Methods returning the the type name if it matches the given type, null or Exception otherwise:

  matchClassType($value, $allowedTypes, bool $throwException = false): ?string;
  matchType(     $value, $allowedTypes, bool $throwException = false): ?string;

Additionally, there are some short-cuts:

• Methods returning the the type name if $value matches the given type(s), or throws an Exception otherwise:

  ensureClassType($value, $allowedTypes): string;
  ensureType(     $value, $allowedTypes): string;

• Methods returning boolean whether $value matches the given type(s):

  isClassType($value, $allowedTypes): bool;
  isType(     $value, $allowedTypes): bool;

Furthermore, I've

• added type constants (see comment)
• improved the methods' documentation

[luke-]

@martin-rueegg Thanks.

I overlooked the fact that the ensure*() methods return a string. Why not void?
I expected the use case e.g.:

function test($param) {
      ensureType($param, ['string']);

      // ...
}

[martin-rueegg]

@martin-rueegg Thanks.

I overlooked the fact that the ensure*() methods return a string. Why not void? I expected the use case e.g.:

function test($param) {
      ensureType($param, ['string']);

      // ...
}

It was only intended as a shortcut for self::matchClassType($value, $allowedTypes, true).

However, I've now changed the return type to void and updated all usages to use matchClassType() instead.

[martin-rueegg]

Currently failing tests seem unrelated to any change in this PR

[luke-]

@martin-rueegg Hmm strange, tests in develop running green at the moment.
https://github.com/humhub/humhub/actions?query=branch%3Adevelop

[martin-rueegg]

@martin-rueegg Hmm strange, tests in develop running green at the moment. https://github.com/humhub/humhub/actions?query=branch%3Adevelop

The error message is really not helpful, stating that some DB fields are not found during Github API test. The issue, however, seem to have been a syntax error in one of the files.

[martin-rueegg]

@luke- Tests successful now. Ready to merge?

[luke-]

@luke- Tests successful now. Ready to merge?

@martin-rueegg Yes, thanks for your effort here!