[4.0] plg_system_debug - Add option to select storage method #22188

elkuku · 2018-09-15T04:51:52Z

Pull Request for Issue #22137 .

Summary of Changes

This will add an option to select a storage type for the debug plugin.

Currently only two options are supported:

File - Debug information will be written to disk in json format.
None

Testing Instructions

Activate debug in global configuration
Switch the storage type in plugin configuration and confirm that files are written - or not.

Expected result

Ability to Set the storage type to 'None'

Actual result

Files are always written - this might be a security issue.

Documentation Changes Required

Yes.
Add words to description of #20380 (TBD)

close #22137

brianteeman · 2018-09-15T08:54:39Z

why is this a new fieldset? Surely it belongs with logging?

elkuku · 2018-09-16T01:23:17Z

I think we should separate this from logging. I believe that 'file' is just the first option and others will follow with additional settings. So the tab will get some more content... Brian Teeman <[email protected]> schrieb am Sa., 15. Sep. 2018, 03:54:

…

why is this a new fieldset? Surely it belongs with logging? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#22188 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACEuuBKhUbLNKbPsEG7LPOWGtKNxcu0ks5ubMBQgaJpZM4WqMqR> .

Quy · 2018-09-16T03:36:34Z

plugins/system/debug/debug.php

 	}
+
+	/**
+	 * Setup the storage method where debug information will be store (or not).


Change to stored

elkuku · 2018-09-16T03:41:30Z

Correct. But I think the sentence is not quite right - the "where" doesn't sound correct. @brianteeman ? Quy <[email protected]> schrieb am Sa., 15. Sep. 2018, 22:36:

…

***@***.**** commented on this pull request. ------------------------------ In plugins/system/debug/debug.php <#22188 (comment)>: > @@ -600,4 +601,26 @@ private function collectLogs(): self return $this; } + + /** + * Setup the storage method where debug information will be store (or not). Change to stored — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#22188 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACEukdee0T712o4gDBYzg8TUIMpMCZrks5ubcdEgaJpZM4WqMqR> .

brianteeman · 2018-09-16T08:32:29Z

Storage only refers to logging so it should be in the logging tab

brianteeman · 2018-09-16T08:54:47Z

It also makes no sense at all what the "storage" is referring to when it is on its own tab.

It needs to be on the logging tab AND only shown when logging is actually enabled

elkuku · 2018-09-17T13:40:41Z

Back on my keyboard...
So, what I am trying to say is that

Logging refers basically to "Messages" that are generated by the system during execution, deprecated messages or messages by 3PD extensions.
I believe activating this should not imply that those messages are going to be written to text files.
Storage refers to the way the whole "debug bar" data (like system info, session and request data, database queries and, well, the log messages) are handled.
Current options include:
- File
- None
  Other possible options would be
- Redis
- PDO
- A custom handler

So, in my humble opinion, this deserves two separated tabs.

On the other hand, if that doesn't make any sense to you, I would be willing, and able, to perform the required changes to get this merged so I can use my keyboard for writing code instead of comments 😉

brianteeman · 2018-09-17T14:17:17Z

please make the requested changes - if the other stuff is ever written then the decision can always be reviewed at that time

elkuku · 2018-09-17T14:41:42Z

Done .

elkuku · 2018-09-17T15:26:09Z

Partially reverted...
If you think that the storage option doesn't deserve its own tab it should go to the general "Plugin" tab, because it has absolutely no connection to the log tab for the reasons stated above.

brianteeman · 2018-09-18T08:31:51Z

I stick by my original comment

mbabker · 2018-09-18T12:13:47Z

This isn't a logging configuration in the sense of dealing with log files, so IMO it's right that it does not go onto the logging tab.

brianteeman · 2018-09-18T12:16:27Z

what is it then? Maybe i have completely misunderstood it but from what I understand its purpose is to decide if log files are created

mbabker · 2018-09-18T12:21:25Z

It's to configure whether the temporary files for a debugged request's data is stored basically, used in conjunction with the debug bar's Open Handler feature. It basically gives you the ability to review data for previous requests.

mbabker · 2018-09-18T12:24:03Z

Well, when this option is fully integrated it wouldn't be "just" whether the data is stored but what backend storage is used (as hinted at earlier in this thread and in another issue).

brianteeman · 2018-09-18T12:25:24Z

Then the word storage is definitely misleading to me although now I see exactly what it is I see it is called storage in the code http://phpdebugbar.com/docs/storage.html

So if the field was called something like "storing collected data" or "collecting data storage" that would be less confusing perhaps?

mbabker · 2018-09-18T12:27:38Z

"Collected Data Storage" should work.

elkuku · 2018-09-18T13:37:49Z

Thank you very much.
Let's blame Mrs. Hamilton, my English teacher at school...
I also could have posted the link to the docs about storage, not sure why I omitted that - My fault.

brianteeman

approve language strings

brianteeman · 2018-09-18T14:16:37Z

Mrs Hamilton did a good job

elkuku · 2018-09-18T15:19:57Z

OK since now we all know what we are talking about... I'd like to pollute my own PR with something a little OT..

I just tried to implement the open handler.
The following code:

# /components/com_content/Controller/DisplayController.php

public function xxx()
{
	$debugBar = new DebugBar();
	$debugBar->setStorage(new FileStorage($this->app->get('tmp_path')));

	$handler = new OpenHandler($debugBar);
	$handler->handle();
}

and

# /plugins/system/debug/debug.php:293

$debugBarRenderer
  ->setOpenHandlerUrl(JUri::root(true) . '/index.php?option=com_content&task=xxx&format=raw');

will enable the browsing of previous requests inside the debug bar which, IMHO, is a killer feature since it will enable the average developer for the first time to examine what happens during a redirect without the requirement for a single step debugger.

Now as you might have noticed the code is "hacked" into com_content which is obviously not the right place.
On the other hand I think that adding a new component (com_debug) to add this functionality would be an overkill...

Suggestions?

mbabker · 2018-09-18T15:30:10Z

Add a view to com_admin.

elkuku · 2018-09-18T23:28:00Z

And for the front end? A simple controller task that returns a JSON response would be enough. Or do we need some overriding strategy here? Michael Babker <[email protected]> schrieb am Di., 18. Sep. 2018, 10:32:

…

Add a view to com_admin. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#22188 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACEuvWGLdtBcXNrlqPddXMoMKFJlZRNks5ucRIQgaJpZM4WqMqR> .

elkuku · 2018-09-20T17:50:51Z

Add a view to com_admin.

I tried hard but I could not find a way to display anything from com_admin in frontend :(
Any hint how to accomplish this would be very much appreciated.

elkuku · 2018-09-24T18:40:43Z

Not sure if we want to explore how to disable this great feature or are we good with the security enhancement that will be introduced by #22327 (which also includes the answer to my OT question here)?
So this could be closed.

joomla-cms-bot · 2018-09-25T05:01:15Z

Set to "closed" on behalf of @franz-wohlkoenig by The JTracker Application at issues.joomla.org/joomla-cms/22188

ghost · 2018-09-25T05:01:16Z

Closed as stated above.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22188.}

Add option to select debug information storage method

fa89ee8

elkuku requested a review from brianteeman as a code owner September 15, 2018 04:51

joomla-cms-bot added Language Change This is for Translators PR-4.0-dev labels Sep 15, 2018

elkuku changed the title ~~[4.0] Add option to select debug information storage method~~ [4.0] plg_system_debug - Add option to select debug information storage method Sep 15, 2018

elkuku changed the title ~~[4.0] plg_system_debug - Add option to select debug information storage method~~ [4.0] plg_system_debug - Add option to select storage method Sep 15, 2018

ghost mentioned this pull request Sep 15, 2018

[4.0] plg_system_debug doesn't log db queries to log files since pr 20380 #22137

Closed

Quy reviewed Sep 16, 2018

View reviewed changes

elkuku added 2 commits September 17, 2018 09:38

Unify tabs

a379be9

Merge remote-tracking branch 'origin/4.0-dev' into plg_debug-storage

46c21af

elkuku added 2 commits September 17, 2018 10:09

Change wording - thanks @Quy

e66f31e

Move storage option to general tab.

5190b3b

Blame npm or PHPStorm but not @elkuku :P

b425740

Change the wording

b40d20e

brianteeman approved these changes Sep 18, 2018

View reviewed changes

joomla-cms-bot closed this Sep 25, 2018

elkuku deleted the plg_debug-storage branch December 20, 2018 15:12

Uh oh!

Uh oh!

[4.0] plg_system_debug - Add option to select storage method #22188

[4.0] plg_system_debug - Add option to select storage method #22188

Uh oh!

Conversation

elkuku commented Sep 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

Uh oh!

brianteeman commented Sep 15, 2018

Uh oh!

elkuku commented Sep 16, 2018 via email

Uh oh!

Quy Sep 16, 2018

Choose a reason for hiding this comment

Uh oh!

elkuku Sep 17, 2018

Choose a reason for hiding this comment

Uh oh!

elkuku commented Sep 16, 2018 via email

Uh oh!

brianteeman commented Sep 16, 2018

Uh oh!

brianteeman commented Sep 16, 2018

Uh oh!

elkuku commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brianteeman commented Sep 17, 2018

Uh oh!

elkuku commented Sep 17, 2018

Uh oh!

elkuku commented Sep 17, 2018

Uh oh!

brianteeman commented Sep 18, 2018

Uh oh!

mbabker commented Sep 18, 2018

Uh oh!

brianteeman commented Sep 18, 2018

Uh oh!

mbabker commented Sep 18, 2018

Uh oh!

mbabker commented Sep 18, 2018

Uh oh!

brianteeman commented Sep 18, 2018

Uh oh!

mbabker commented Sep 18, 2018

Uh oh!

elkuku commented Sep 18, 2018

Uh oh!

brianteeman left a comment

Choose a reason for hiding this comment

Uh oh!

brianteeman commented Sep 18, 2018

Uh oh!

elkuku commented Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbabker commented Sep 18, 2018

Uh oh!

elkuku commented Sep 18, 2018 via email

Uh oh!

elkuku commented Sep 20, 2018

Uh oh!

elkuku commented Sep 24, 2018

Uh oh!

joomla-cms-bot commented Sep 25, 2018

Uh oh!

ghost commented Sep 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

elkuku commented Sep 15, 2018 •

edited

Loading

elkuku commented Sep 17, 2018 •

edited

Loading

elkuku commented Sep 18, 2018 •

edited

Loading