Pronovix / password_enhancements

0 stars 3 forks source link

Remove unneeded complexity around `\Drupal\password_enhancements\Type\QueryOrder` #41

Closed boobaa closed 4 years ago

boobaa commented 5 years ago

It's never actually used; the only "real" entry point is in \Drupal\password_enhancements\Entity\Storage\PolicyEntityStorage::getEntityIdsByRoleAndPriority().

This also removes the need for Drupal\password_enhancements\Type\Type.

ycecube commented 5 years ago

It was introduced to make sure that it gets the correct type, since it is not possible to define enums and require enum like types as a parameters, so this is the closest that can emulate this.

The functions that uses the QueryOrder type can be called by the user, so if someone would extend the module wouldn't be able to accidentally pass a wrong value for the function.

This also eliminates the need to validate the possible values.

boobaa commented 5 years ago

I think it's way too much jazz/noise for a simple decision of the order things are returned. What about replacing those entry points with a (bool) $asc = TRUE or something like that?

ycecube commented 5 years ago

I wanted to introduce this structure here now I've seen the possibility. I have missing the enum typehint since php7.0, okay there is no enum either in php, but I think it is still a great way to define the possible values for a function/method's argument where it comes to multiple possibilities. Sadly it has to be emulated, but I don't think that it would add too much overhead.

By doing this you are basically eliminating the need for checking the correct argument's options, also there is no need for extra error handling because by design it can only accept the predefined options. It is also easier to extend/change it if needed, and it makes the code more readable.

It is true that in this case it would work using boolean to make sure the correct value is passed, but what if we extend the module and there will be a method/function that for example could have four or more possible values for an argument? If you use string then the possible values would have to be validated, handled, and in case of a mismatch the error should be handled, same goes for integers, we could also define constants for the function/method, but again that would be prone to user error if someone would like to use the module's functions/methods. So by using a structure like this, you would just have to extend the Drupal\password_enhancements\Type\Type abstract class and define only your possible argument options on your custom type class.

I understand that this may be a bit of an overkill for one type, but I was expecting to use this structure in the future on other places as well, and hopefully in other modules too eventually.

If PHP sometime in the future would pull in the enum types and enum typehints we could easily replace it because the name of the type wouldn't have to be changed at all (okay maybe the value handling needs to be changed if they restrict the enum values to integer).

boobaa commented 4 years ago

Closed by #57.