Problem/Motivation

This comment in services.yml is rather confusing and could use better wording.

It can read like it's not recommended to set the `cache: true` value. But instead, what it's trying to say is that it is not recommended to disable cache on a production environment.

Steps to reproduce

Visit the services.yml provided by Drupal core
https://git.drupalcode.org/project/drupal/-/blob/8.9.x/sites/default/def...

Proposed resolution

Better wording so it's not confusing when reading the comment.

Remaining tasks

Update comments.

CommentFileSizeAuthor
#13 interdiff_12-13.txt1.26 KBravi.shankar
#13 3166449-13.patch3.21 KBravi.shankar
#12 3166449-12.patch3.21 KBravi.shankar
#7 interdiff_6-7.txt1.13 KBmmatsoo
#7 correct-period-position-in-improved-wording-31664499-7.patch3.2 KBmmatsoo
#6 interdiff-3-6.patch4.03 KBadamzimmermann
#6 improve-wording-twig-cache-setting-prod-environment-comment-3166449-6.patch3.2 KBadamzimmermann
#3 improve-wording-twig-cache-setting-prod-environment-comment-3166449-3.patch1.57 KBwalangitan

Comments

joshua.boltz created an issue. See original summary.

cilefen’s picture

cilefen commented
Issue tags: +Documentation
walangitan’s picture

walangitan commented
Status: Active » Needs review
StatusFileSize
new improve-wording-twig-cache-setting-prod-environment-comment-3166449-3.patch1.57 KB

Took a pass at updating the comments to include enabling/disabling the values. I was looking to see if there is already a pattern for more detailed comments about setting values and am curious what the community thinks is best to help clarify these. I do agree that the current comment:

Not recommended in production environments

is not as clear as it should be.

Post Tests Edit: This is failing from this test, so there is additional work to remedy once there's a decision to the wording.

1) Drupal\Tests\ComposerIntegrationTest::testExpectedScaffoldFiles with data set #18 ('sites/default/default.services.yml', 'assets/scaffold/files/default...es.yml')
Scaffold source and destination files must have the same contents.
Failed asserting that two strings are equal.

Status: Needs review » Needs work

The last submitted patch, 3: improve-wording-twig-cache-setting-prod-environment-comment-3166449-3.patch, failed testing. View results

ayushmishra206’s picture

ayushmishra206 commented
Issue tags: +Needs tests
adamzimmermann’s picture

adamzimmermann commented
Status: Needs work » Needs review
StatusFileSize
new improve-wording-twig-cache-setting-prod-environment-comment-3166449-6.patch3.2 KB
new interdiff-3-6.patch4.03 KB

I like the new wording. While we are at it, can we add a "." to the end of the sentences?

I attached a new patch that handles this and should fix the failing test as well.

mmatsoo’s picture

mmatsoo commented
StatusFileSize
new correct-period-position-in-improved-wording-31664499-7.patch3.2 KB
new interdiff_6-7.txt1.13 KB

@adamzimmermann I noticed a couple of typos while reviewing your patch. ("environme.nts") Attached is a re-rolled patch.

adamzimmermann’s picture

adamzimmermann commented

@mmatsoo thank you!

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev
star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs review » Needs work
Issue tags: -Needs tests +Novice

Documentation might be a better component than cache system but going to leave it for now. I don't think this needs tests.

Otherwise, looking pretty good, I would suggest replacing "cacheability metadata" with "cacheability debugging", I think it would be more accurate, and also consistent with the "header" of # Cacheability debugging: above that setting.

ravi.shankar’s picture

ravi.shankar commented
StatusFileSize
new 3166449-12.patch3.21 KB

Added reroll of patch #7 on Drupal 9.3.x.

ravi.shankar’s picture

ravi.shankar commented
Status: Needs work » Needs review
StatusFileSize
new 3166449-13.patch3.21 KB
new interdiff_12-13.txt1.26 KB

Made change as per comment #11.

star-szr’s picture

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

Looks good to me, thanks @ravi.shankar (and of course everyone else who has worked on this so far)!

  • catch committed 1f171b9 on 10.0.x 
    Issue #3166449 by ravi.shankar, adamzimmermann, mmatsoo, walangitan,...

  • catch committed 5f3489f on 9.4.x 
    Issue #3166449 by ravi.shankar, adamzimmermann, mmatsoo, walangitan,...

  • catch committed c029706 on 9.3.x 
    Issue #3166449 by ravi.shankar, adamzimmermann, mmatsoo, walangitan,...
catch’s picture

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

Committed 1f171b9 and pushed to 10.0.x. Thanks!
Also cherry-picked to 9.4.x and 9.3.x.

Status: Fixed » Closed (fixed)

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