Multiple problems with /admin/store/orders/%/log [#2647802]

The most obvious problem is that all the log messages show escaped HTML.
The other problem is that the User column is always empty.
And a big WTF is that the individual log items are each wrapped in <div><ul><li> ... </li></ul></div> then saved to the database with all that extra HTML. Note that a log item is at most one entry, so there is only one bullet each.

Here's a patch that fixes the rendering of the page so that we see the User column and we don't see escaped HTML. At the same time, it fixes the saving of the log entries so that extra HTML is not added and not stored in the DB.

Comment	File	Size	Author
#3	order-log2.patch	3.55 KB	TR
#3	8.x-4.x: PHP 5.5 & MySQL 5.5, D8.1 171 pass
	order-log.patch	3.53 KB	TR
	8.x-4.x: PHP 5.5 & MySQL 5.5, D8.1 149 pass, 32 fail

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

11 January 2016 at 00:26

TR created an issue. See original summary.

Comment #2

11 January 2016 at 00:37

Status:

Needs review

» Needs work

The last submitted patch, order-log.patch, failed testing.

Comment #3

TR CreditAttribution: TR commented 11 January 2016 at 01:03

Status:

Needs work

» Needs review

File	Size
order-log2.patch	3.55 KB
8.x-4.x: PHP 5.5 & MySQL 5.5, D8.1 171 pass

Comment #4

11 January 2016 at 02:04

TR committed 5073a14 on 8.x-4.x

Issue #2647802 by TR: Multiple problems with /admin/store/orders/%/log

Comment #5

TR CreditAttribution: TR commented 11 January 2016 at 02:08

Status:

Needs review

» Fixed

Committed.

Comment #6

longwave

he/him

English

CreditAttribution: longwave commented 11 January 2016 at 14:25

       foreach ($changes as $key => $value) {
         if (is_array($value)) {
-          $items[] = t('@key changed from %old to %new.', ['@key' => $key, '%old' => $value['old'], '%new' => $value['new']]);
+          $entry = t('@key changed from %old to %new.', ['@key' => $key, '%old' => $value['old'], '%new' => $value['new']]);
         }
         else {
-          $items[] = (string) $value;
+          $entry = (string) $value;
         }
       }

I was sure there was a case somewhere where this was called with multiple values; at the moment if $changes contains more than one value then only the last one is stored, as $entry is overwritten on each pass of the loop.

Comment #7

TR CreditAttribution: TR commented 13 January 2016 at 00:03

I had intended to move the db_insert() inside the loop. Here's a follow-up patch to do that. EDIT: I accidentally committed it before I posted it here (commit f859b9cd), and it got mixed up with something else I was doing so it has the wrong commit message. Sigh.

logChanges() really should be scrapped and we should use the logger service instead to write to our own table. But it works such as it is and there are much more important to work on right now.