Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Debug Toolbar Enhancements #910

Merged
merged 11 commits into from
Apr 13, 2018

Conversation

natanfelles
Copy link
Contributor

Now is possible get the Toolbar in HTML (debug bar), JSON and XML.

Accessing an existing file like http://localhost:8080/index.php?debugbar_time=1516260538 will return the content by the Accept Header, according with content negotiation, default is HTML.

The Toolbar data is an array saved with json_encode. When a request is done the negotiation occurs.

Yet is not working with the API Response. But this will facilitate the process.

$contents = file_get_contents($filename);
unlink($filename);
$contents = self::format(file_get_contents($filename), $format);
//unlink($filename); // TODO - Keep history? 10 files
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment the files are not being deleted, so that tests can be done by accessing the URLs directly.

Thinking about the possibility of maintaining a history of requests. Initially just keeping some files that can be handled later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a history, besides being able to show it in the debug bar, also could be possible add a custom Header or append a link to the debug data.

I was thinking about being able to standardize API responses. For example:

{
	"meta":
	{
		"code": 200,
		"pagination":
		{
			"next": ""
		}
	},
	"data": {},
	"debug": {}
}

Since the the response body is inside the data. It would be nice to manipulate debugbar and already show some data by default. But this could overfill the output.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@natanfelles natanfelles force-pushed the toolbar_formats branch 2 times, most recently from 2569ab8 to 98f6897 Compare January 18, 2018 11:45
@natanfelles natanfelles changed the title Saves the encoded debugbar data and enables decoding by content type Debug Toolbar Enhancements Jan 20, 2018
@natanfelles
Copy link
Contributor Author

The code is a little messy. But this is a concept to meet the issue #83 .

I made a video, not listed here: https://www.youtube.com/watch?v=NqqyKVVc0qQ

Please tell us if this is the desired direction and what should be changed.

@lonnieezell
Copy link
Member

I think it’s looking great! I haven’t had a chance to look over the code but did watch the video. Like it a lot.

One thing I would add is a setting in the config file for the max number of requests to save. 0 would be unlimited and any positive value would delete any files older than that number.

Great job! Thanks

Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few smaller things that need changing, but all in all looks pretty good. Would you say you're done with the feature aside from these changes?


$output = '';

if ($format === 'html')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be nicer as a "switch". A bit simpler to read than a series of if/elseifs.

'isEmpty' => ! (bool)$count,
'hasTabContent' => true,
'hasLabel' => true,
'icon' => '<img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABgAAAAYCAYAAADgdz34AAAAAXNSR0IArs4c6QAAAARnQU1BAACxjwv8YQUAAAAJcEhZcwAADsMAAA7DAcdvqGQAAAJySURBVEhL3ZU7aJNhGIVTpV6i4qCIgkIHxcXLErS4FBwUFNwiCKGhuTYJGaIgnRoo4qRu6iCiiIuIXXTTIkIpuqoFwaGgonUQlC5KafU5ycmNP0lTdPLA4fu+8573/a4/f6hXpFKpwUwmc9fDfweKbk+n07fgEv33TLSbtt/hvwNFT1PsG/zdTE0Gp+GFfD6/2fbVIxqNrqPIRbjg4t/hY8aztcngfDabHXbKyiiXy2vcrcPH8oDCry2FKDrA+Ar6L01E/ypyXzXaARjDGGcoeNxSDZXE0dHRA5VRE5LJ5CFy5jzJuOX2wHRHRnjbklZ6isQ3tIctBaAd4vlK3jLtkOVWqABBXd47jGHLmjTmSScttQV5J+SjfcUweFQEbsjAas5aqoCLXutJl7vtQsAzpRowYqkBinyCC8Vicb2lOih8zoldd0F8RD7qTFiqAnGrAy8stUAvi/hbqDM+YzkAFrLPdR5ZqoLXsd+Bh5YCIH7JniVdquUWxOPxDfboHhrI5XJ7HHhiqQXox+APe/Qk64+gGYVCYZs8cMpSFQj9JOoFzVqqo7k4HIvFYpscCoAjOmLffUsNUGRaQUwDlmofUa34ecsdgXdcXo4wbakBgiUFafXJV8A4DJ/2UrxUKm3E95H8RbjLcgOJRGILhnmCP+FBy5XvwN2uIPcy1AJvWgqC4xm2aU4Xb3lF4I+Tpyf8hRe5w3J7YLymSeA8Z3nSclv4WLRyFdfOjzrUFX0klJUEtZtntCNc+F69cz/FiDzEPtjzmcUMOr83kDQEX6pAJxJfpL3OX22n01YN7SZCoQnaSdoZ+Jz+PZihH3wt/xlCoT9M6nEtmRSPCQAAAABJRU5ErkJggg==">',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be just the value of the icon itself. Leave the presentation to the front, since it could be used as an img src, or a background in CSS, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All Collectors Icons are being saved with the debugbar file.

One approach could be to define them here to show the icons only in the HTML output. But all icons would have to be set in the template, which would ignore the icon() method defined there in classes.

Or are you referring to remove the img tag and leave only the base64? I didn't quite understand.

$rows = $this->collectTimelineData();
$files = [];

$current = Services::request()->getGet('debugbar_time');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please bring the request object in through the constructor and saved as a class variable. Using the Services class within libraries is frowned upon typically.

for ($i = 0; $i < $total; $i++)
{
// Oldest files will be deleted
if ($i >= 30)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make the value here set in Config\App. Some people may have semi-sensitive information they don't want kept around long on their servers during testing, so they should be able to define the amount to keep around.

@natanfelles
Copy link
Contributor Author

Yes. I think is done, aside the requested changes.

I'll see if I make the changes until tomorrow, but if you have time now and you want to improve this, feel free. 👍

@lonnieezell lonnieezell merged commit 4d87f68 into codeigniter4:develop Apr 13, 2018
@byazrail
Copy link
Contributor

Collectors/Views::icon() img src bug.
History tab bug;
Argument 2 passed to CodeIgniter\View\Parser::prepareReplacement() must be of the type string, null given, called in system/View/Parser.php on line 580

@natanfelles
Copy link
Contributor Author

@byazrail Please, clean your debug files in the WRITEPATH and say if it solves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants