Problem/Motivation

See #2566503: [meta] Replace remaining !placeholder for Non-URL HTML outputs only

modules/syslog/syslog.module:      $output .= '<p>' . t("The Syslog module logs events by sending messages to the logging facility of your web server's operating system. Syslog is an operating system administrative logging tool that provides valuable information for use in system management and security auditing. Most suited to medium and large sites, Syslog provides filtering tools that allow messages to be routed by type and severity. For more information, see the <a href='!syslog'>online documentation for the Syslog module</a>, as well as PHP's documentation pages for the <a href='!php_openlog'>openlog</a> and <a href='!php_syslog'>syslog</a> functions.", array('!syslog' => 'https://www.drupal.org/documentation/modules/syslog', '!php_openlog' => 'http://www.php.net/manual/function.openlog.php', '!php_syslog' => 'http://www.php.net/manual/function.syslog.php')) . '</p>';
modules/syslog/syslog.module:    '#description'   => t('Specify the format of the syslog entry. Available variables are: <dl><dt><code>!base_url
Base URL of the site.
!timestamp
Unix timestamp of the log entry.
!type
The category to which this message belongs.
!ip
IP address of the user triggering the message.
!request_uri
The requested URI.
!referer
HTTP Referer if available.
!uid
User ID.
!link
A link to associate with the message.
!message
The message to store in the log.

'),

Proposed resolution

Remaining tasks

Agree that removing HTML support makes sense.

User interface changes

None

API changes

Date format strings no longer support adding HTML using the \ escape character.

Data model changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because at the moment date formats support HTML but it is escaped
Issue priority Major because part of the critical to remove !placeholder
Disruption Disruptive for existing sites that are adding HTML to date formats. If HTML is required in a formatted date then the site should implement a custom field formatter to do this.
CommentFileSizeAuthor
#23 Mozilla_Firefox.png28.96 KBjoelpittet
#5 remove_placeholder_in-2571953-5.patch4.4 KBtom verhaeghe
#6 interdiff-5-6.txt723 bytestom verhaeghe
#6 remove_placeholder_in-2571953-6.patch5.1 KBtom verhaeghe

Comments

dawehner created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
commented
Issue summary: View changes
dawehner’s picture

dawehner
Primary language German
commented
Issue summary: View changes
tom verhaeghe’s picture

tom verhaeghe commented
Assigned: Unassigned » tom verhaeghe
tom verhaeghe’s picture

tom verhaeghe commented
Assigned: tom verhaeghe » Unassigned
Status: Active » Needs review
StatusFileSize
new remove_placeholder_in-2571953-5.patch4.4 KB

Not sure if this patch makes sense, but here's my attempt. All of the variables passed on by the syslog function are preceded with @ now instead of an exclamation mark.

tom verhaeghe’s picture

tom verhaeghe commented
StatusFileSize
new remove_placeholder_in-2571953-6.patch5.1 KB
new interdiff-5-6.txt723 bytes

Forgot to alter a test in the migrate_drupal module that refers to this change. This will probably make the test fail :-)

Status: Needs review » Needs work

The last submitted patch, 5: remove_placeholder_in-2571953-5.patch, failed testing.

The last submitted patch, 6: remove_placeholder_in-2571953-6.patch, failed testing.

tom verhaeghe’s picture

tom verhaeghe commented
Status: Needs work » Needs review
tom verhaeghe’s picture

tom verhaeghe commented
Status: Needs review » Needs work
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Reviewed & tested by the community 
+++ b/core/modules/syslog/src/Logger/SysLog.php
@@ -79,15 +79,15 @@ public function log($level, $message, array $context = array()) {
+      '@link' => strip_tags($context['link']),

That feels a bit iffy but it doesn't cause double escaping due to the strip_tags(), right? What happens with a quote in @link or @message?

alexpott’s picture

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

I'm not convinced about this change. Since the !placeholder vs @placeholder is irrelevant here - we are calling strtr() not SafeMarkup::format() or t().

I think this is closed won't fix.

dawehner’s picture

dawehner
Primary language German
commented

IMHO its a best practise now, as otherwise it would add a bit of confusion.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

So if we do this then we have to fix the migration.

+++ b/core/modules/migrate_drupal/src/Tests/Table/d7/Variable.php
@@ -582,7 +582,7 @@ public function load() {
-      'value' => 's:72:"!base_url|!timestamp|!type|!ip|!request_uri|!referer|!uid|!link|!message";',
+      'value' => 's:72:"@base_url|@timestamp|@type|@ip|@request_uri|@referer|@uid|@link|@message";',

Making this change is wrong.

cosmicdreams’s picture

cosmicdreams commented

@alexpott, for my education: Why is that change wrong?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Because that is the Drupal 7 default value and we're testing how we migrate that to drupal 8.

stefan.r’s picture

stefan.r commented 
+++ b/core/modules/syslog/syslog.module
@@ -54,7 +54,7 @@ function syslog_form_system_logging_settings_alter(&$form, FormStateInterface $f
-    '#description'   => t('Specify the format of the syslog entry. Available variables are: !base_url !timestamp etc'),
+    '#description'   => t('Specify the format of the syslog entry. Available variables are: @base_url @timestamp etc'),

What about this one - we still use !variable here but I assume this is something else entirely as we don't pass in an argument?

stefan.r’s picture

stefan.r commented
Priority: Critical » Major

OK so that is used in strtr() only. Seems this has nothing to do with the parent so this is at best not critical and probaby just a won't fix, as I don't see how using @ instead of ! there is less confusing?

stefan.r’s picture

stefan.r commented
Parent issue: #2566503: [meta] Replace remaining !placeholder for Non-URL HTML outputs only »

The last submitted patch, 5: remove_placeholder_in-2571953-5.patch, failed testing.

The last submitted patch, 6: remove_placeholder_in-2571953-6.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Category: Bug report » Task

It is also not a bug.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Category: Task » Bug report
Issue summary: View changes
Issue tags: +rc target triage
StatusFileSize
new Mozilla_Firefox.png28.96 KB

This needs to happen or we'll have some serious problems with 8.0.0 #2575703: Remove default fall-through from PlaceholderTrait::placeholderFormat()

It's now a bug:

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Category: Bug report » Task

Whoops, that was coming from devel module.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Closed (works as designed)

I think there is no point changing the syslog format. It is not the same thing as the placeholders for FormattableMarkup cause it is not markup.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: -rc target triage