Problem/Motivation

REQUEST_TIME is a deprecated global. There are several issues to remove it's use in core.

This issue is to remove it from just Functional and FunctionalJavascript tests.

Proposed resolution

Replace REQUEST_TIME usages in Functional and FunctionalJs tests with \Drupal::time()->getRequestTime() where appropriate.

Remaining tasks

Check that the unresolved issues in the first MR are resolved.
Update the latest MR, the diff no longer applies.

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3309104

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mondrake created an issue. See original summary.

mondrake opened merge request !2753

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
mondrake’s picture

mondrake
Location 🇮🇹
commented
Assigned: mondrake » Unassigned
daffie’s picture

daffie commented
Status: Needs review » Needs work

Why is the change to the file core/scripts/dev/commit-code-check.sh is the PR? Should that not get its own issue?

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

#6: reverted

daffie’s picture

daffie commented

All the changes look good to me.

Just one question: Should we change the "REQUEST_TIME" in web/core/modules/dblog/tests/src/Functional/FakeLogEntries.php too?

mondrake’s picture

mondrake
Location 🇮🇹
commented

#8: yes, but not yet - a Kernel test core/modules/dblog/tests/src/Kernel/DbLogTest.php is calling generateLogEntries() too, and it would fail with the change ATM since the new property is only defined for functional tests. Let's do that in the parent when we close up.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

@mondrake: Thank you for your reply.

All changes look good to me.
All instances in FunctionalTests have been changed.
For me it is RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

I'm not convinced about adding another test property. There are so many. Why didn't we use \Drupal::time() everywhere? Even if we had private services everywhere I'd have thought we might keep this one public.

ravi.shankar made their first commit to this issue’s fork.

ravi.shankar’s picture

ravi.shankar commented

There are more instances where we need to remove testRequestTime and use \Drupal::time()

bhanu951’s picture

bhanu951 commented
Version: 10.0.x-dev » 11.x-dev

Bhanu951 opened merge request !4316

Bhanu951 closed merge request !4316

Bhanu951 opened merge request !4317

bhanu951’s picture

bhanu951 commented

Created New MR #4317 against 11.x Branch and Removed testRequestTime property and used drupal time in remaining places.

bhanu951’s picture

bhanu951 commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left a comment but the issue summary mentions this should be FunctionalJavascript file but the MR I see 1 FunctionalJavascript file changed. So MR and issue summary don't line up.

bhanu951’s picture

bhanu951 commented
Status: Needs work » Needs review

@smustgrave

I ran grep -R "REQUEST_TIME" core >list.txt and found no additional FunctionalJavascript tests with REQUEST_TIME.

So, The title seems correct.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

In that case the changes to the functional tests should be reverted as they would be out of scope

bhanu951’s picture

bhanu951 commented
Status: Needs work » Needs review

@smustgrave

Seems you mis-read, this issue is to fix both Functional and FunctionalJavascript tests.I will update Title and Summary. So back to NR.

bhanu951’s picture

bhanu951 commented
Title: Replace REQUEST_TIME in Functional/FunctionalJavascript tests » Replace REQUEST_TIME in Functional and FunctionalJavascript tests
Issue summary: View changes
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Applied the MR and searched phpstan-baseline for REQUEST_TIME which had 59 matches. None for Functional or FunctionalJavascript files.

quietone’s picture

quietone commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

This needs a reroll.

I skimmed both MR and I am not convinced that the comments by alexpott in the first MR have been addressed and answered. The proposed resolution is a bit different than what it implemented. It looks like the proposed resolution was for when a new property was being added.

I've updated the remaining tasks. Back to NW.

acbramley made their first commit to this issue’s fork.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review

Feedback addressed, pipeline's green again. Hiding the old MR.

acbramley changed the visibility of the branch 3309104-replace-requesttime-in to hidden.

acbramley’s picture

acbramley commented
Issue summary: View changes
Parent issue: #3112284: Replace REQUEST_TIME in Kernel tests » #2902895: [meta][no patch] Replace uses of REQUEST_TIME and time() with time service
Related issues: +#3112284: Replace REQUEST_TIME in Kernel tests
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Applied the MR

Searching phpstan-baseline remaining REQUEST_TIME and all instances of FunctionalJavascript and Functional appear to be replaced.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

One question on the MR, everything else looks good.

acbramley’s picture

acbramley commented
Issue summary: View changes
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Believe the one thread from @catch has been answered.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Let's add a comment explaining what's happening there.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review

acbramley credited mstrelan.

acbramley’s picture

acbramley commented
Status: Needs review » Reviewed & tested by the community

Going to self RTBC since this was a simple change and we went with @mstrelan's version.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

  • catch committed e5e61dc5 on 11.x 
    Issue #3309104 by Bhanu951, acbramley, mondrake, ravi.shankar,...
catch’s picture

catch
he/him
Primary language English
commented

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

mondrake closed merge request !2753