Support from Acquia helps fund testing for Drupal Acquia logo

Comments

willvincent’s picture

Status: Active » Needs review
FileSize
1.68 KB

Here's a patch that adds this functionality to the available rules system actions.

Rory’s picture

This was useful, thanks.

Severity and link should probably both be optional, i.e.

        'severity' => array(
          'type' => 'integer',
          'label' => t('Severity'),
          'options list' => 'watchdog_severity_levels',
          'optional' => TRUE,
        ),
        'link' => array(
          'type' => 'text',
          'label' => t('An associated, HTML formatted link'),
          'optional' => TRUE,
        ),

And be NULL arguments by default:

/**
 * Action: Create a watchdog entry.
 */
function rules_action_watchdog($type, $message, $severity = NULL, $link = NULL) {
  ...
}
mitchell’s picture

Title: Add a watchdog action for D7 » Action: Create a watchdog entry
Component: Provided Rules integration » Rules Engine
Status: Needs review » Needs work

Per #2.

wjaspers’s picture

Status: Needs work » Needs review
FileSize
1.76 KB

Actually, the severity is required. It defaults to the notice severity, WATCHDOG_NOTICE.
For reference: http://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/watch...

Here's an updated patch.

EDIT: Updated comment to re-issue test request.

Rory’s picture

It is not required on the form - hence the addition of the 'optional' attribute.

mitchell’s picture

Component: Rules Engine » Rules Core
Priority: Normal » Major
Status: Needs review » Needs work
FileSize
2.02 KB

#4 works.

It would be better to use description fields for the text currently in the labels. I tried to edit #4 directly to fix that, but the resulting patch I'm uploading is malformed. Please update #4 soon so we can get this committed.

pluess’s picture

git apply returns
fatal: corrupt patch at line 63
for the patch of #6

mitchell’s picture

^ "the resulting patch I'm uploading is malformed"
My notes and patch in #6 go with the needs review > needs work change. If you'd like to help roll a patch, I'd appreciate the help.

Use #4 if you need this now.

pluess’s picture

Status: Needs work » Needs review
FileSize
2.38 KB

Sorry, I didn't read your notes carfully. Please find a new patch attached including your changes.

And thanks for the new action. It does what it's supposed to do: Add watchdog entries.

Xen’s picture

I've just tested this patch, and it does what it says on the tin...

mitchell’s picture

Status: Needs review » Reviewed & tested by the community

#9, #10: Awesome.

klausi’s picture

Status: Reviewed & tested by the community » Needs work

This action should use only one parameter with the data type "log_entry". Same as the watchdog event uses.

mitchell’s picture

I think @klausi is saying that the action here should reuse _rules_system_watchdog_log_entry_info(). @previous-patch-submitters, please provide a new patch based on this provided data type?

mitchell’s picture

Status: Needs work » Postponed

I started to work on this based on #12, but then realized there were more advantages to solving this in Entity API. Therefore, I propose we resolve this in #1714486: Add wrappers for watchdog's log entries. (Changing status to follow up there.)

@fago, I believe in #682722: What other modules can be candiadte for entity API, you said that this approach was "questionable" but not undesirable, and I think I've described some reasonable benefits.

I'm still progressing with php, so this patch was giving me some trouble, but I'd be happy to try again. If anyone with more experienced has the interest in getting this done, I would appreciate your efforts greatly.

sonicthoughts’s picture

+1

Angry Dan’s picture

Status: Postponed » Reviewed & tested by the community

Seems to me like the bigger issue of using entity API to wrapper the watchdog isn't getting solved any time soon. I'd say that we should commit the patch in #9 now.

Angry Dan’s picture

Priority: Major » Normal

... But not with "major" priority

PatchRanger’s picture

I'm using it and it works for me too.
As Angry Dan I'm in doubt about getting entity-wrapped log entry soon - so I support the suggestion to commit this patch.

mitchell’s picture

Better Watchdog UI (formerly Watchdog Entity) would only need small changes to complete the entity wrapping. See #2040213: Rules Watchdog Integration.

fago’s picture

Status: Reviewed & tested by the community » Needs work

ad #12: I think it's ok to go without the data structure as the creating the data structure first would just pose UX problems without giving any extra functionality or features.

The patch looks good (did not test it though). It misses test coverage though - we should test it in the Rules core integration test case, where all other system actions are tested. E.g. execute the action and make sure the watchdog entry has been written.

Also, what about replacements? Replacements are the only way to create proper translatable watchdog entries so we should support providing them. E.g. just add two list parameter for replacement keys and values? The nicer variant would be probably making a message module - watchdog bridge but that's another story.

mrded’s picture

Status: Needs work » Needs review
Anybody’s picture

Issue summary: View changes

The patch works good for me! It would be cool to see this feature in the next release :)

sonicthoughts’s picture

+1 for inclusion in next release.

Summit’s picture

+1 for inclusion in 2.8! greetings, Martijn

Summit’s picture

Status: Needs review » Reviewed & tested by the community
hanoii’s picture

For what it worth, better_watchdog_ui module has a watchdog rules action.

EDIT: Oops, it was mentioned above, sorry.

fago’s picture

Status: Reviewed & tested by the community » Needs work

see #20.

MustangGB’s picture

Issue tags: +Needs tests
sano’s picture

the patch #9 did not work for me. After installing it I see a new action "Log a system message", but the action config screen contains only the Save button.
The patch applied without problems, but I wonder if the new code could be colliding with other patches I applied to the Rules module in the past. If this is the case, this just points to the troubles we expose ourselves, if patches do not get committed (fairly quickly) - folks just develop/test new patches against different code bases.

TR’s picture

this just points to the troubles we expose ourselves, if patches do not get committed (fairly quickly)

@sano: The maintainers of Rules said it wasn't ready to commit, and listed the things that still needed to be done (see e.g. #20). No one in the community worked on those things, so the patch for this new feature never got finished.

It's NOT the case in this instance that a completed patch has been sitting around ready to commit - this is an incomplete feature request with known things missing from it. In #29 you yourself found that the patch doen't work, so I don't see why you think it would have been a good idea to commit this long ago.

If you want to see this feature implemented and committed, step 1 is to update the patch so that it applies to the current version of Rules, fix the problems that you've identified, then most importantly address the issues raised by the maintainers. When we have a patch that does all this, THEN it can be reviewed and committed. That hasn't happened yet.

sano’s picture

@TR I understand and agree with all you are writing. When I wrote what I wrote I had two issues on my mind:

1. The other Rules patches I have installed work "on my system", but the developers and potential testers of this patch might not be using them, or have installed various subsets of that collection. Therefore different people might get different testing results. Committing the "other" patches and therefore creating a common code base to test this patch against would make the situation manageable. So it is not necessarily this patch that should have been committed, but the other ones. Of course all of this might be a red herring and the problem with this patch might be unrelated to my concern.

2. When I read the #20 requirement, the last section caught my attention: "...Also, what about replacements? Replacements are the only way to create proper translatable watchdog entries so we should support providing them. E.g. just add two list parameter for replacement keys and values? The nicer variant would be probably making a message module - watchdog bridge but that's another story...."
I agree that this would be a nice to have feature, but not critical for the original requirement stated in the opening of this thread. The condition of its implementation before the patch can be committed IMHO makes that event more unlikely. I feel without that requirement the number of people willing to work on this issue might be higher.

I would like my comment not to be viewed as a critique - it is more of a lament about the general state of the FOSS development, caused by IMHO a bad incentive structure for the right people* to devote attention to - for example - completing patches like this one. I think we need to start rewarding developers for their work economically and with a better reputation reward... but that is theme for a different discussion thread and forum**.

* I am really not very good php programmer, which explains the lack of a patch attached to my comments here.
** which one? I have a proposal ready, for addressing this issue.

TR’s picture

TR’s picture

#2170013: Provide system action to write a db watchdog message has a better patch that what has been posted here so far, but we still need to address @fago's comments in #20.

TR credited jonathan1055.

TR’s picture

Here's the patch from #2170013-2: Provide system action to write a db watchdog message, renamed and rerolled to apply against HEAD. Credit goes to jonathan1055 for this patch.

TR’s picture

Once more, with the 1 new coding standards problem fixed.

web226’s picture

Thanks @TR your patch at #36 worked perfectly for me with Drupal 7.65 and PHP 7.2 and Rules 7.x-2.12

bmango’s picture

The patch in #36 worked great. Many thanks!

  • TR committed aa4ce78 on 7.x-2.x
    Issue #1349882 by pluess, jonathan1055, TR: Action: Create a watchdog...
TR’s picture

Status: Needs work » Patch (to be ported)

Committed. Needs to be ported to D8 now. The D8 action should have tests!

TR’s picture

Version: 7.x-2.x-dev » 8.x-3.x-dev

Should be 8.x-3.x now ...

gregori.goossens’s picture

Hi,

patch #36 work for me, but i use it for write messages generated with parameters in watchdog log and due to t() function on "message" log entry Drupal create a translatable string for each different message.
Maybe we must found a solution for send variables array from Rules to watchdog callback to prevent multiple record in translation table.

What do you think about it ?

Thanks

TR’s picture

@gregori.goossens:

due to t() function on "message" log entry Drupal create a translatable string for each different message

Where in the code is this t() function? The patch above doesn't wrap the $message in t().

I can fix this problem if I know where the wrong code is ...

If you change this code in the drupal_watchdog action in modules/system.rules.inc:

        'message' => array(
          'type' => 'text',
          'label' => t('Message'),
          'description' => t('The text of the message.'),
          'translatable' => TRUE,
        ),

to use 'translatable' => FALSE, does that fix the problem?

gregori.goossens’s picture

The t() function is later when message is displayed.

Maybe we can use a generic text (that could be translated later) in $message with an additional variables array.

ex :
$message = "My generic message in report : %param1 at %param2"
$variables = ["custom message 1","2020-01-30"];

with something like :

function rules_action_drupal_watchdog($type, $message, $variables, $severity, $link_text, $link_path) {
  if (!empty($link_path)) {
    // Use $link_path for the text if no specific text was supplied.
    $link = l(!empty($link_text) ? $link_text : $link_path, url($link_path));
  }
  else {
    $link = NULL;
  }

  $variables_named = array();
  if(!empty($variables)) {
    foreach ($variables as $key => $variable) {
      $variables_named["!param" . ($key+1)] = $variable;
    }
  }

  watchdog($type, $message, $variables_named, $severity, $link);
}

and $variables define in rules action info

        'variables' => array(
          'type' => 'list<text>',
          'label' => t('Variables List', array(), array('context' => 'data_types')),
          'description' => t('The list with variables text use it in message with !param1, !param2 etc... .'),
          'optional' => TRUE,
        ),
gagarine’s picture

Status: Patch (to be ported) » Needs work

I don't understand the state of this feature in D8.

What is #2762157: [meta] Log to the logging service?

The action "Create a watchdog entry" is implemented for D8? It doesn't look like it (I don't see the feature in the admin).