When a 301 is returned to the client, we should log the action and let the site owner know to update the requesting URL with the new one.

CommentFileSizeAuthor
#21 restclient-log-redirects-2178943-21-D7.patch3.69 KBCptCasual
#18 restclient-log-redirects-2178943-18-D7.patch3.32 KBCptCasual
#15 restclient-log-redirects-2178943-15-D7.patch3.25 KBCptCasual
#13 restclient-log301-2178943-13-D7.patch2.62 KBCptCasual
#11 restclient-log301-2178943-11-D7.patch2.07 KBCptCasual
#7 restclient-log301-2178943-7-D7.patch1.63 KBCptCasual
#2 restclient-log301-2178943-0-D7.patch1.26 KBCptCasual

Comments

minoroffense’s picture

minoroffense commented
Assigned: Unassigned » CptCasual
CptCasual’s picture

CptCasual commented
StatusFileSize
new restclient-log301-2178943-0-D7.patch1.26 KB

Added log for 301.
Restclient logs now take a parameter for the log severity level.

CptCasual’s picture

CptCasual commented
Status: Active » Needs review
minoroffense’s picture

minoroffense commented

First, when you submit a patch you should set it to "Needs Review" in the status.

minoroffense’s picture

minoroffense commented
Status: Needs review » Needs work

Next, a few notes:

  1. Instead of just 301s, let's do this on any 3XX class response code. There's a function in the module that returns which class of response code. Use that.
  2. The message in the log should not be empty. Return something like "Redirection response when calling @url" or something like that. Explain why this is in the log.
minoroffense’s picture

minoroffense commented

When you have a new patch, set the status back to "needs review"

CptCasual’s picture

CptCasual commented
StatusFileSize
new restclient-log301-2178943-7-D7.patch1.63 KB

Changed to log all 3XX response codes.
Modified the message. Note that the message is formatted in _restclient_watchdog.

Questions about if (variable_get('restclient_watchdog', TRUE))
1. Should it be checked on line 804? Or be moved inside _restclient_watchdog so the callers don't have to check it?
2. Would it be more correct to reverse the logic as: if (!variable_get('restclient_watchdog', FALSE))

CptCasual’s picture

CptCasual commented
Status: Needs work » Needs review
minoroffense’s picture

minoroffense commented
Status: Needs review » Needs work

1. Yeah moving that into the function makes sense. Do that.
2. What's the advantage to reversing the login? Are we changing the default behavior? If so we'll need to notify users and decide if it's worth it.

CptCasual’s picture

CptCasual commented

Currently, if the restclient_watchdog config var is not present then the logs are on (TRUE) by default.
(Oops, comment 7 question 2 should not have the NOT.)

CptCasual’s picture

CptCasual commented
Status: Needs work » Needs review
StatusFileSize
new restclient-log301-2178943-11-D7.patch2.07 KB

Moved the check for logging config into the log function.

CptCasual’s picture

CptCasual commented
Assigned: CptCasual » minoroffense

Assigning to minorOffense for review.

CptCasual’s picture

CptCasual commented
StatusFileSize
new restclient-log301-2178943-13-D7.patch2.62 KB

Updated the admin config text since Watchdog logs are not just for errors now.

CptCasual’s picture

CptCasual commented
Title: Log 301 responses to watchdog as warnings » Log 3XX redirect responses to watchdog
CptCasual’s picture

CptCasual commented
StatusFileSize
new restclient-log-redirects-2178943-15-D7.patch3.25 KB

I had not added 3xx logs for POST and PUT. Fixed now.

IMPORTANT QUESTION: 5xx responses do not get logged because $response->error is not set. Should 5xx also get logged?

CptCasual’s picture

CptCasual commented
minoroffense’s picture

minoroffense commented
Assigned: minoroffense » CptCasual
Status: Needs review » Needs work

Yeah we could do 5XX responses too.

Only code change I would make is change the text to read "Enable logging" and add a #description to the form saying "Log error, redirect or not found responses".

CptCasual’s picture

CptCasual commented
Assigned: CptCasual » minoroffense
Status: Needs work » Needs review
StatusFileSize
new restclient-log-redirects-2178943-18-D7.patch3.32 KB

Changed the config text, per review.

Have not implemented logging for 5xx.
Mat: Would you like me to create a new issue for logging 5xx?

minoroffense’s picture

minoroffense commented
Assigned: minoroffense » Unassigned
Status: Needs review » Reviewed & tested by the community

Yeah create a new issue

minoroffense’s picture

minoroffense commented
Assigned: Unassigned » CptCasual
Status: Reviewed & tested by the community » Needs work

Can you rebuild this patch against the latest dev. Also, be sure to add a variable_del('restclient_watchdog') in hook_uninstall (see restclient.install for examples).

CptCasual’s picture

CptCasual commented
StatusFileSize
new restclient-log-redirects-2178943-21-D7.patch3.69 KB

Rebuilt patch against 7.x-2.x.
Added variable_del('restclient_watchdog') in hook_uninstall.

CptCasual’s picture

CptCasual commented

@minorOffense : Does this stay as a patch or should I request a merge?

minoroffense’s picture

minoroffense commented

If you have a working branch you can create the merge request in Gitlab.

CptCasual’s picture

CptCasual commented
Status: Needs work » Needs review

  • spotzero committed 20fea82 on 7.x-2.x authored by CptCasual 
    Issue #2178943 by CptCasual: Log 3XX redirect responses to watchdog
spotzero’s picture

spotzero commented
Status: Needs review » Fixed

Fixed.

Status: Fixed » Closed (fixed)

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