Not a bug \XF\Service\User\TempChange applies temporary changes even if $expiryDate is in the past

[DragonByte Tech]

Affected version

2.0.9

I realise this is something that can be handled by each addon that uses this service, but in my opinion this is still considered a bug because it makes more sense to have this check in the service itself, as opposed to forcing every addon to make the check.

In src/XF/Service/User/TempChange.php find

        if ($changeKey !== null)
        {
            $this->expireUserChangeByKey($user, $changeKey);
        }

Add below:

        if ($expiryDate && $expiryDate < \XF::$time)
        {
            $this->db()->commit();
            return null;
        }

This could potentially be a breaking change if an addon relied on the XF:UserChangeTemp entity that was returned, so maybe it would be worth returning an unsaved XF:UserChangeTemp entity instead.


Fillip

 

[Chris D]

I realise this is something that can be handled by each addon that uses this service, but in my opinion this is still considered a bug because it makes more sense to have this check in the service itself, as opposed to forcing every addon to make the check.

My response to that, would be, why would the expiry date be in the past, and therefore why is such a check necessary? Presumably, even if it did happen, the temp change would be expired within an hour anyway.

Even so, I'm not sure the best approach is to involve the TempChange service here at all, vs. just making sure you don't attempt to apply a change that is logically flawed in the first place.

Happy to consider a change here or alternatives if I can understand the use case better.

 

[DragonByte Tech]

I'm not sure the best approach is to involve the TempChange service here at all, vs. just making sure you don't attempt to apply a change that is logically flawed in the first place.

Happy to consider a change here or alternatives if I can understand the use case better.

I consider the change to be useful under the DRY principle - forcing every 3rd party developer to wrap their calls to the service in logic checks needlessly repeats code that could easily be added to the TempChange service and would not cause any BC / functionality breakage.

Is there a reason not to add this check to the service?

The use case I hit when using this service was in our eCommerce mod. I recently added a new feature where product purchases could apply temporary as well as permanent user group changes, and as part of the license rebuild service I wanted to apply temporary changes. Some licenses may be expired, yet those users would still receive the temporary change for - as you say - up to an hour.

Sure, I could add a check for the expiry date before running the service, but then so would everyone else using the service rather than have the service itself do the check, needlessly repeating code.

I’m just not understanding why it would be a bad thing to add the check to the service, could you elaborate please?


Fillip

 

[Chris D]

I’m just not understanding why it would be a bad thing to add the check to the service, could you elaborate please?

It would be remiss of us to add code without fully understanding why that code is needed.

Even now, I'd argue that hardly anyone would need to add code to check the expiry date as I don't feel like adding temp changes retrospectively would be a common use case, at all.

Even putting that to one side, it does kind of break existing code on the basis that the return value expected from this method is a saved UserChangeTemp entity. Yes, we could return an unsaved entity, but that then breaks existing assumptions and would require developers to still write code to check if the received object is saved before working with it. Imagine trying to save the UserChangeTemp user_change_temp_id (which would be null for an unsaved entity) to a database that is expecting an integer; that code would start falling over with entity or MySQL errors unexpectedly.

So, on the basis that it can break existing code and/or require extra code checks put in place anyway, all thing's considered - just validate the expiry date of the change in your own code if you need to (most people don't).