Project

General

Profile

Actions

Task #97815

closed

Excessive logging in TypoScriptFrontendController set_no_cache

Added by Rasmus Sallling almost 2 years ago. Updated about 1 year ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
Frontend
Start date:
2022-06-27
Due date:
% Done:

0%

Estimated time:
TYPO3 Version:
12
PHP Version:
Tags:
Complexity:
easy
Sprint Focus:

Description

We're seeing an excessive amount of set_no_cache triggered warnings, caused by setting no_cache.

See the code in question here:
https://github.com/TYPO3/typo3/blob/main/typo3/sysext/frontend/Classes/Controller/TypoScriptFrontendController.php#L2404

After having looked through the issue history around this feature, I've found no real discussion around why this is logged as a WARNING as opposed to just a NOTICE (which could be reasonably suppressed in production). Therefore I'm submitting this, if nothing else then to understand the basis for this.

Short summary:
TYPO3 will, when set_no_cache is called, issue a log WARNING that contains either a caller provided reason for why it was called or alternatively a pointer to what code called it, generated by calling the backtrace function.
In addition to the plain set_no_cache logging case, there is also the related but somewhat separate "disableNoCacheParameter" handling that logs as a WARNING if set_no_cache is logged, but is ignored by the [TYPO3_CONF_VARS][FE][disableNoCacheParameter]. This also triggers a WARNING.

Argument:
Essentially I believe this is overuse of WARNING. While triggering these logs certainly is an indicator of bad design, this is not universally the case. It seems much more sane to simply log this as a notice, so that people can enable it when needed, which would mostly be during development or during debugging.

Alternatively one could of-course provide the means to suppress these logs, but that feels like excessive configurability. The logging system has a logtype designed for this type of thing, and I believe that is a NOTICE in this case.

Also, changing the log type to NOTICE would also allow us to deprecate the ugly "internal" variable, which does exactly what I'm suggesting for internal calls, and which seems like mis-design to have on a public function.


Related issues 1 (0 open1 closed)

Related to TYPO3 Core - Task #100067: Streamline logging in TypoScriptFrontendController->set_no_cacheClosedThomas Hohn2023-03-03

Actions
Actions #1

Updated by Thomas Hohn about 1 year ago

  • Tracker changed from Bug to Task
  • Is Regression deleted (No)
Actions #2

Updated by Gerrit Code Review about 1 year ago

  • Status changed from New to Under Review

Patch set 1 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/78011

Actions #3

Updated by Gerrit Code Review about 1 year ago

Patch set 2 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/78011

Actions #4

Updated by Gerrit Code Review about 1 year ago

Patch set 3 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/78011

Actions #5

Updated by Gerrit Code Review about 1 year ago

Patch set 4 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/78011

Actions #6

Updated by Gerrit Code Review about 1 year ago

Patch set 5 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/78011

Actions #7

Updated by Christian Kuhn about 1 year ago

  • Related to Task #100067: Streamline logging in TypoScriptFrontendController->set_no_cache added
Actions #8

Updated by Thomas Hohn about 1 year ago

The variable $internal was renamed to $internalRequest in order reflect the purpose.
It's not possible to get rid of the variable since it is indeed used with the
value false from a core call. Will close it since #100067 has been merged.

Actions #9

Updated by Thomas Hohn about 1 year ago

  • Status changed from Under Review to Closed
Actions

Also available in: Atom PDF