The ChangedItem field type is using REQUEST_TIME. It should use the time service instead.

CommentFileSizeAuthor
#2 using-time-service-in-changeditem-2902896-2.patch1.07 KBquocnht
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

quocnht’s picture

quocnht CreditAttribution: quocnht as a volunteer commented
Status: Active » Needs review
FileSize
using-time-service-in-changeditem-2902896-2.patch1.07 KB

Replaced REQUEST_TIME with \Drupal::time()->getRequestTime()

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Title: replace REQUEST_TIME with time service in ChangedItem » Replace REQUEST_TIME with time service in ChangedItem
Status: Needs review » Reviewed & tested by the community

Looks good to me.

joachim’s picture

joachim CreditAttribution: joachim commented

Shouldn't the time service be injected?

quocnht’s picture

quocnht CreditAttribution: quocnht as a volunteer commented

@joachim: #2053415: Allow typed data plugins to receive injected dependencies

joachim’s picture

joachim CreditAttribution: joachim commented

That's type data rather than field type though isn't it?

I can see several field type plugins that say:

> 177: // @todo Inject entity storage once typed-data supports container injection.

but equally, core/modules/field/src/Plugin/migrate/process/FieldType.php does it by implementing ContainerFactoryPluginInterface.

quocnht’s picture

quocnht CreditAttribution: quocnht as a volunteer commented

core/modules/field/src/Plugin/migrate/process/FieldType.php is a MigrateProcessPlugin process field_type.

FieldType plugin in field system extends DataType plugin.

joachim’s picture

joachim CreditAttribution: joachim commented

Ohhh. Whoops! I was searching for files with FieldType in the name...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: using-time-service-in-changeditem-2902896-2.patch, failed testing. View results

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs work » Reviewed & tested by the community

Bot fluke.

  • catch committed eb0da10 on 8.5.x 
    Issue #2902896 by quocnht, joachim: Replace REQUEST_TIME with time...

  • catch committed 7c4b828 on 8.4.x 
    Issue #2902896 by quocnht, joachim: Replace REQUEST_TIME with time...

catch credited catch.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

cilefen’s picture

cilefen CreditAttribution: cilefen as a volunteer commented

ContentEntityChangedTest is failing a lot on HEAD.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Fixed » Needs work

Reverted from both branches for now.

  • catch committed 6691587 on 8.5.x 
    Revert "Issue #2902896 by quocnht, joachim: Replace REQUEST_TIME with...

  • catch committed 310735a on 8.4.x 
    Revert "Issue #2902896 by quocnht, joachim: Replace REQUEST_TIME with...
quocnht’s picture

quocnht CreditAttribution: quocnht as a volunteer commented

Maybe it related to : #2857843: Random fail in Drupal\KernelTests\Core\Entity\ContentEntityChangedTest::testChanged.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

c31ck’s picture

c31ck CreditAttribution: c31ck commented
Status: Needs work » Needs review

Now that #2857843 has been fixed, we might be able to move forward again with this issue. Patch from #2 still applies cleanly, I've triggered the retest and it does not have any failures.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community

RTBC based on #10 and #3. Should we try to figure out if this issue was the problem for the failures discovered trough #15 or do we think that #2857843: Random fail in Drupal\KernelTests\Core\Entity\ContentEntityChangedTest::testChanged fixed this?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Closed (duplicate)

Thank you for your work on cleaning up Drupal core's use of deprecated APIs!

In order to deprecate APIs in a maintainable way, converting deprecated uses should be replaced across all of core for a given kind of usage, rather than in individual modules or files. Such issues should also always be part of an overall plan to ensure all usages are removed, rather than standalone patches.

For background information on why we usually will not commit cleanups that aren't scoped in that way, see the core issue scope guidelines. See the core deprecation policy for more information on how we handle deprecations.

Contributing to the overall plan above will help ensure that your cleanups for core's deprecated code improve core in a maintainable and minimally disruptive way.

Marking this as a duplicate of #2902895: [meta][no patch] Replace uses of REQUEST_TIME and time() with time service where we'll come up with a plan that's not class by class, file by file.