Misleading parameter name $expire in KeyValueStoreExpirableInterface

Are these two interfaces related in some way? It's not obvious to me why they can't both use the parameter name $expire to mean different things. If a class would be likely to implement both, then they'd have two methods with nearly the same name with different meanings, but otherwise... Requiring that the same parameter name has the same meaning across all uses of it would be very difficult to enforce!

Definitely we should not use $ttl as a variable name, as it violates our coding standards for variable names (abbreviations are bad, writing out words is good). So $lifetime would be a better variable/parameter name. And I don't have a problem with changing the variable name... just make a patch. :)

This is still an issue.

I think it makes sense to rename this parameter to $time_to_live, to avoid confusion about it being a timestamp (as it is in other contexts). Good Novice issue?

Note that it needs to be changed in the Interface and any classes that implement the method (there are two).

On it.

Here we go.

Oops, sorry. There are actually 3 methods that have the problem :-). On it.

Looks good to me!

Note: this is really just a docs change. Changes name of parameter to several methods, with no change in functionality.

Hm, I disagree that this is only a documentation change. While there is indeed no impact on calling code, changing a parameter name does have a small risk of adding a functional bug. That said, the existing parameter name is misleading, which adds some fragility, so I am considering this a prioritized change per: https://www.drupal.org/core/beta-changes#prioritized

Since the potential for disruption is very minimal, I this is okay to go into 8.0.x. Let's add a beta evaluation.

I think we also need to name the other places that the time-to-live for key value stores is named $expire. For example, see the protected property SharedTempStore::$expire (there are many similar cases).

Oops.

Changing class member names would be actual API changes, though, as opposed to changing the parameter name in a method, which (if it's done right) does not change the API. Is it prioritized enough that this would be a good idea at this stage in the game?

Well, my thought is that it's worse to change only some references to the same quantity than either changing all of them or leaving all of them consistent. Changing a protected property name is a limited API break, but if it's too disruptive, then I don't think we should do it halfway.

A beta eval of the specific BC breaks (in things that we would add to the patch) will probably help us decide.

OK then, sounds like we need a new patch and a beta evaluation. And since this is not just docs any more, I'm bowing out of the discussion.

xjm: maybe you could indicate whether you think it's even worth proceeding, since you seem unwilling to have the non-disruptive patch committed, and it's not clear the disruptive one is OK to commit? It would be annoying for someone to waste time doing a patch if it's just going to get thrown out. Or maybe we can just do the non-disruptive fix?

Re-rolled the patch from #8 against 8.2.x. The additional changes in #10 still need to be addressed.

Updating issue status to trigger automated tests.

Let's assign this back to @xjm to decide what to do, based on #10-#14.

From a long-term software developer's point of view, I'd say that the change away from $expire and to $time_to_live (or $time_to_timeout, as expire implies it's time left, not time to commencing) is a very sensible idea. If this is a setting that can be set through an external interface (e.g. the API), rather than just inside the method etc, then that should also be changed.

As a Novice Sprinter, though a very experienced software developer, I'm not sure what else I can add at this stage, other than running code searches to see where this code is called from and listing all those places ready for changes to be made.

I spoke to @Virbius about this issue. Whether or not we still go forward with this depends on the specific things we will need to change. We've adopted the policy that protected properties and methods are considered internal, so we can rename them if needed to fix bugs. We can also do something like deprecate the old name and add the new one, if it's too disruptive to remove it. But the best way to decide will be to get the full scope of what might be affected. So @Virbius will do some research to see where else this might apply.

I pointed out that $time_to_live is a correct label since that's the standard terminology in caching. (The confusion here was the difference between "live" as a verb and as an adjective, I think.)

Thanks everyone!

I am working on this. Will update the patch soon

Hi @xjm, @virbius,

I have updated the patch for 9.1. Please have a look

Path is working fine. Moving it to RTBC.

Thanks for finding this unresolved issue and bringing it up to date.

#21 has still not been addressed.

• We need to look at other places in core where it's still called $expire, including member variables, local variables, and other APIs.
• We need to identify which should be changed for consistency.
• If we rename member variables, etc., we should do so using deprecation for backwards compatibility.

Please make sure to read the issue and work to address the feedback rather than rerolling patches that have already been marked "Needs work". I'm removing credit for rerolls that did not incorporate the feedback. You can get credit on this issue by helping to address the needed changes. Thanks!

Hi @xjm,
$expire is used in 25 files in core and here I will attach the screenshot of the file's data. Please let me know the next step of this issue.

In regards to #36, there are some files that are `$expires` and not purely `$expire` so we must be a bit cautious with replacing these. Looking at these and working on a patch that makes updates recommended from #21.

First pass on from the feedback xjm left on #21, #34 and the callouts from #36 and #37

This does not address or update the database definitions in the semaphore table in DatabaseLockBackend.php, left this alone in this patch, perhaps another issue entirely, just note for those searching for $expire. Awaiting the automated test results as I expect that some issues will be identified that will need to be addressed.
Variables are being renamed here and as xjm said, we should do so using deprecation for backwards compatibility, note that this is affecting a large number of variables and was not included in this patch.

Test cases are failing, Test Result (3,310 failures / +3302). Moving this to needs work.

re-rolled for 10.1.x