In this ticket we should introduce the minimal version of webform_submission_log.

It would basically encompass adding the module itself, moving the table {webform_submission_log} under its jurisdiction, migrating the WebformSubmissionStorage::log() method to using actual Drupal logger and then implementing a logger service within webform_submission_log module that would listen for 'webform_submission_log' channel only.

CommentFileSizeAuthor
#36 3015180-37.patch76.06 KBjrockowitz
#36 interdiff-3015180-33-37.txt14.83 KBjrockowitz
#33 interdiff-29-33.txt4.21 KBbucefal91
#33 3015180-log-submodule-33.patch64.98 KBbucefal91
#29 interdiff-26-29.txt1.25 KBbucefal91
#29 3015180-log-submodule-29.patch62.96 KBbucefal91
#26 3015180-25.patch62.64 KBjrockowitz
#26 interdiff-3015180-24-25.txt5.06 KBjrockowitz
#25 interdiff-3015180-23-24.txt9.08 KBjrockowitz
#25 3015180-24.patch57.94 KBjrockowitz
#23 3015180-23.patch58.08 KBjrockowitz
#23 interdiff-3015180-22-23.txt24.63 KBjrockowitz
#22 3015180-22.patch63.79 KBjrockowitz
#22 interdiff-3015180-21-22.txt15.79 KBjrockowitz
#21 interdiff-3015180-20-21.txt9.09 KBjrockowitz
#21 3015180-21.patch57.69 KBjrockowitz
#20 3015180-20.patch52.26 KBjrockowitz
#17 3015180-17.patch52.19 KBjrockowitz
#17 interdiff-3015180-16-17.txt15.08 KBjrockowitz
#16 3015180-16.patch41.18 KBjrockowitz
#16 interdiff-3015180-14-16.txt782 bytesjrockowitz
#14 3015180-14.patch40.22 KBjrockowitz
#14 interdiff-3015180-13-14.txt22.11 KBjrockowitz
#13 interdiff-11-13.txt764 bytesbucefal91
#13 3015180-log-submodule-13.patch20.09 KBbucefal91
#11 interdiff-9-11.txt8.09 KBbucefal91
#11 3015180-log-submodule-11.patch20.09 KBbucefal91
#9 interdiff-7-9.txt2.02 KBbucefal91
#9 3015180-log-submodule-9.patch12.97 KBbucefal91
#7 3015180-log-submodule-7.patch10.96 KBbucefal91
#7 interdiff-5-7.txt1.85 KBbucefal91
#5 3015180-log-submodule-5.patch11.14 KBbucefal91
#3 3015180-log-submodule-2.patch11.27 KBbucefal91
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bucefal91 created an issue. See original summary.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 commented
Status: Active » Needs review

Here's the minimal necessary amount of code to introduce webform_submission_log module with an update path.

if this patch looks OK and get committed, as a next step I would refactor all logging related methods out from WebformSubmissionStorage class and would place it all within webform_submission_log module. Also, I'd refactor the tests so webform_submission_log submodule actually gets reasonable test coverage.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 commented
FileSize
3015180-log-submodule-2.patch11.27 KB

here's the patch. phpunit tests passed on my localhost. Let's see what simpletests will say on d.org testbox infrastructure.

Status: Needs review » Needs work

The last submitted patch, 3: 3015180-log-submodule-2.patch, failed testing. View results

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 commented
Status: Needs work » Needs review
FileSize
3015180-log-submodule-5.patch11.14 KB

Let's see if this one applies.

Status: Needs review » Needs work

The last submitted patch, 5: 3015180-log-submodule-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 commented
Status: Needs work » Needs review
FileSize
interdiff-5-7.txt1.85 KB
3015180-log-submodule-7.patch10.96 KB

Let's see if it passes this way.

Status: Needs review » Needs work

The last submitted patch, 7: 3015180-log-submodule-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 commented
Status: Needs work » Needs review
FileSize
3015180-log-submodule-9.patch12.97 KB
interdiff-7-9.txt2.02 KB

This should get us a green light.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented

All the submission_log settings in the UI need to have #access added to the element.
@see \Drupal\webform\Form\AdminConfig\WebformAdminConfigSubmissionsForm
@see \Drupal\webform\EntitySettings\WebformEntitySettingsSubmissionsForm

'#access' => $this->moduleHandler->moduleExists('webform_submission_log')

Maybe the webform_scheduled_email.module should require the webform_submission_log.module.

\Drupal\webform\Entity\Webform::hasSubmissionLog should check that the webform_submission_log.module exists.

The webform_update_8154() module should only enable the webform_submission_log module if the submission log is enabled globally or on any webform or webform_scheduled_email.module is installed.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 commented
FileSize
3015180-log-submodule-11.patch20.09 KB
interdiff-9-11.txt8.09 KB

Hey, Jake! :)

Things look more calm now in my life so here I am, trying to finish up this ticket.

I have implemented your feedback except for one point:

\Drupal\webform\Entity\Webform::hasSubmissionLog should check that the webform_submission_log.module exists.

My idea was a little bit different. Let user enable/disable logging on the webform. Then it definitely gets logged into watchdog and in case webform_submission_log is enabled, it also gets logged into the dedicated table. Therefore in my vision ::hasSubmissionLog() wouldn't require the webform_submission_log submodule to be enabled.

Does it make sense? I do not insist on my vision, I just want to confirm which way you prefer it?

And I am attaching a next iteration patch for your review.

Status: Needs review » Needs work

The last submitted patch, 11: 3015180-log-submodule-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 commented
Status: Needs work » Needs review
FileSize
3015180-log-submodule-13.patch20.09 KB
interdiff-11-13.txt764 bytes

This should get us full test pass.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
FileSize
interdiff-3015180-13-14.txt22.11 KB
3015180-14.patch40.22 KB

The patch is great. I am excited to get this committed so I decided to help complete a few more remaining tasks.

Changes

  • Moved webform and webform_submission log delete to webform_submission_log.module.
  • Moved all log related routes and links into the webform_submission_log.module.
  • Moved 'access webform submission log' permission into th webform_submission_log.module.
  • Moved log controller into the webform_submission_log.module
  • Moved submission and node log tests into the webform_submission_log.module

TDB

  • Move if ($entity->getWebform()->hasSubmissionLog()) into WebformSubmissionStorage::log.

I completely agree with…

My idea was a little bit different. Let user enable/disable logging on the webform. Then it definitely gets logged into watchdog and in case webform_submission_log is enabled, it also gets logged into the dedicated table. Therefore in my vision ::hasSubmissionLog() wouldn't require the webform_submission_log submodule to be enabled.

For websites that permanently retain all watchdog logs, they might not need or want the dedicated webform_submission log module or table.

If the 'webform_submission_log' is always enabled we need to consider the somewhat duplicate messages. We also need to be aware the 'webform_submission_log' messages include email addresses.

I am tempted to update the 'Enable submission log' checkboxes help text to state that more verbose and detailed message will be logged in 'Recent log messages'. We can also recommend that users enable the webform_submission_log.module.

Status: Needs review » Needs work

The last submitted patch, 14: 3015180-14.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Status: Needs work » Needs review
FileSize
interdiff-3015180-14-16.txt782 bytes
3015180-16.patch41.18 KB

Next steps

  • Allow logging to be enabled without the webform_submission_log.module
  • Update 'Log submission events' help text to include 'Submission logs will track more detailed user information including email addresses and subjects.'
  • Add dismissiable info message to 'Log all submission events' (/admin/structure/webform/config/submissions) recommending user enble the 'Webform Submission Log' module to better track and permanantely store submission logs.

Todo

  • Update the below methods to do basic or verbose logging based on $webform_submission->getWebform()->hasSubmissionLog()
  • \Drupal\webform\WebformSubmissionStorage::doPostSave
  • \Drupal\webform\Plugin\WebformHandler\EmailWebformHandler::sendMessage
  • \Drupal\webform_scheduled_email\WebformScheduledEmailManager::schedule

I can make these changes and write up a manual test script.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
FileSize
interdiff-3015180-16-17.txt15.08 KB
3015180-17.patch52.19 KB

I think the next big change is to create a dedicate 'webform_submission' log channel and then change

\Drupal::logger('webform_submission_log')->info($message, $context);

to

\Drupal::logger('webform_submission')->info($message, $context);

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented

Ultimately we should be replace calls to and then deprecate \Drupal\webform\WebformSubmissionStorage::log

Status: Needs review » Needs work

The last submitted patch, 17: 3015180-17.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Status: Needs work » Needs review
FileSize
3015180-20.patch52.26 KB

Fixing broken test.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
FileSize
3015180-21.patch57.69 KB
interdiff-3015180-20-21.txt9.09 KB

Changes

  • Created a dedicated 'webform_submission' log channel.
  • Started using 'webform_submission' log channel
  • Create $context for all calls to the 'webform_submission' log.
  • Added channel parameter to \Drupal\webform\Plugin\WebformHandlerBase::getLogger($channel)

Next steps

Explore deprecating \Drupal\webform\WebformSubmissionStorage::log and \Drupal\webform\Plugin\WebformHandlerBase::log

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
FileSize
interdiff-3015180-21-22.txt15.79 KB
3015180-22.patch63.79 KB

This patch now calls the webform_submission log directly and use notice instead of info.

Deprecated
\Drupal\webform\WebformSubmissionStorage::log
\Drupal\webform\Plugin\WebformHandlerBase::log

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
FileSize
interdiff-3015180-22-23.txt24.63 KB
3015180-23.patch58.08 KB

The attached patch has some basic API cleanup and some additional code review.

Status: Needs review » Needs work

The last submitted patch, 23: 3015180-23.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
FileSize
3015180-24.patch57.94 KB
interdiff-3015180-23-24.txt9.08 KB

Final tweaks

  • FIxed broken test.
  • Made sure all logged messages are prefixed with the webform submission's label.
  • When feasible log the same message to channels
  • Removed webform_submission_log dependency from webform_scheduled_email.module

Test script

  • Purge all watchdog logs (/admin/reports/dblog/confirm)
  • Create a test 'Contact' webform submission (/webform/contact/test)
  • Confirm 'webform' log recent message (/admin/reports/dblog?type%5B%5D=webform)
  • 'Enable log submission' events for 'Contact' webform (/admin/structure/webform/manage/contact/settings/submissions)
  • Create a test 'Contact' webform submission (/webform/contact/test)
  • Confirm 'webform_submission' recent log message (/admin/reports/dblog?type%5B%5D=webform_submission)
  • Enable 'Webform Submission Log' module (/admin/modules)
  • Create a test 'Contact' webform submission (/webform/contact/test)
  • Confirm 'webform_submission' recent log message (/admin/reports/dblog?type%5B%5D=webform_submission)
  • Confirm global 'Submissions: Log' (/admin/structure/webform/submissions/log)
  • Confirm webform 'Results: Log' (/admin/structure/webform/manage/contact/results/log)
  • Confirm webform submission 'Log' (/admin/structure/webform/manage/contact/submission/SID/log)
jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Status: Needs work » Needs review
FileSize
interdiff-3015180-24-25.txt5.06 KB
3015180-25.patch62.64 KB

…and I forgot to move all the log related help to the webform_submission_log.module.

Status: Needs review » Needs work

The last submitted patch, 26: 3015180-25.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Status: Needs work » Needs review
bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 commented
FileSize
3015180-log-submodule-29.patch62.96 KB
interdiff-26-29.txt1.25 KB

Hi, Jacob!
:)

That's a lot of diff right there in those patches :)

Mostly I am following you, but there are a few spots that I want to clarify:

In WebformScheduledEmailManager why do we sort of 'dynamically' pick up logging channel?

      $channel = ($webform->hasSubmissionLog()) ? 'webform_submission' : 'webform'; // <===== this line
      $context = [
        '@title' => $webform_submission->label(),
        '@action' => $action,
        '@handler' => $handler->label(),
        '@date' => $send_iso_date,
        'link' => $webform_submission->toLink($this->t('View'))->toString(),
        'webform_submission' => $webform_submission,
        'handler_id' => $handler_id,
        'operation' => 'email ' . $action,
      ];
      $this->getLogger($channel)->notice("@title: Email @action by '@handler' handler to be sent on @date.", $context);

My understanding was that from any point in code we can just do \Drupal::logger('webform_submission')->info('Some message', ['webform_submission' => $webform_submission]); and it would land into watchdog and (optionally) into {webform_submission_log} table as long as logging is enabled on that webform.

There is similar "game" with 'webform' and 'webform_submission' channels in EmailWebformHandler. I am not against it but neither I can say I actually understand why :) I see WebformSubmissionStorage::doPostSave() also may log into 2 different channels depending on the ::hasSubmissionLog() output.

In WebformSubmissionLogLogger I believe it is better to write into DB untrasnlated string. I actually now understand I just silently did this change and did not bring it up earlier in this ticket and it was something worth discussing and stressing out. dblog writes it untranslated too. If we write untranslated string, we then can present the event in multiple languages (because translation happens at the time of displaying and not at the time of writing into DB). Also, in case translation is updated, the new translation becomes reflected on the submission log pages. In my original patch (comment #13) I was using 'data' column to store message placeholders and untranslated message in the 'message' column.

And there are a few smaller things that I actually went ahead and coded:

  • WebformSubmissionLogController::webformStorage had wrong @var comment.
  • WebformSubmissionLogLogger in its class comment carried outdated log channel of 'webform_submission_log' while with the recent patches of yours it has become 'webform_submission'.
jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented

In WebformScheduledEmailManager why do we sort of 'dynamically' pick up logging channel?

By default, everything goes into the webform channel. Having a dedicated webform_submission channel allows people to isolate webform_submission transactions, which may contain user data, without the webform_submission_log .module. In some cases, the same message is logged in the webform and webform_submission channel. In the EmailWebformHandler, the user's email address is being logged in the 'webform_submission' channel and not the 'webform channel'.

In WebformSubmissionLogLogger I believe it is better to write into DB untranslated string.

I agree, but we should add a variables columns, similar to watchdog. The data column is intended to store arbitrary data, possible the submission's serialized data or a remote posts response. This use case for the data column needs to be documented.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 commented

I see. With the explanation of why do we use 2 channels it makes sense.

I will add the variables column either today after working hours or tomorrow and will post a patch here. :)

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Assigned: Unassigned » bucefal91

Thanks. Reassigning this to @bucefal91

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Ocelot commented
Assigned: bucefal91 » Unassigned
FileSize
3015180-log-submodule-33.patch64.98 KB
interdiff-29-33.txt4.21 KB

Here's an iteration that adds 'variables' column and makes use of it to store translation variables so the message gets translated at the moment of displaying and not at the moment of DB write.

Update hook has this 'strange' woodoo ritual of creating a null-allowed column, writing an empty string to it, altering the column and making it not null. That's the most elegant way I can think of to add a not null blob/text column to a pre-existing table. I tested the update path to the best of my capacity and it works.

Cheers!

Status: Needs review » Needs work

The last submitted patch, 33: 3015180-log-submodule-33.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented

I will fix the tests now.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Status: Needs work » Needs review
FileSize
interdiff-3015180-33-37.txt14.83 KB
3015180-37.patch76.06 KB

Change includes…

  • Populating variable column with an empty serialized array
  • Set default submission log values.
  • Creating \Drupal\webform_submission_log\Tests\WebformSubmissionLogTrait
  • Fix tests

@bucefal91 Next week, I am planning on tagging the first stable release of the webform for D8 and I would like to get this merged in.

The last step is creating the change record. Do you have time available to do it?

It could be very simple like "Webform Submission Log moved to submodule." and list the deprecated methods with a code snippet or two.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Ocelot commented

Yowhoo! Stable release!

Yes, I will put up a change record now.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Ocelot commented

There we go https://www.drupal.org/node/3020595 I probably made it too developer-friendly. If you think that's too much of details, feel free to throw away all that dev gibberish :)

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Status: Needs review » Fixed

  • jrockowitz committed 3f9f3ad on 8.x-5.x 
    Issue #3015180 by jrockowitz, bucefal91: Add 'webform_submission_log'...
  • jrockowitz committed 44fbe96 on 8.x-5.x
    Issue #3015180 by jrockowitz, bucefal91: Add 'webform_submission_log'...

Status: Fixed » Closed (fixed)

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