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

Improve error_log & error pass-back; Add getTicket(); Silence verbose logging; Link to repo #42

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

lsthompson
Copy link
Collaborator

@lsthompson lsthompson commented Jun 9, 2023

Helps as the class gets added to code-bases, easier to track it. You weren't wrong about not working with it!

Investigate @danhunsaker concept of PSR-3 handling mentioned in Issue #19 back in 2015.

https://www.php-fig.org/psr/psr-3/

Helps as the class gets added to code-bases, easier to track it
@lsthompson lsthompson changed the title Add link to repo & remove (c) year Improve error_log with prefixes; Add link to repo & remove (c) year from script Jun 14, 2023
@lsthompson lsthompson changed the title Improve error_log with prefixes; Add link to repo & remove (c) year from script Improve error_log & error pass-back; Add getTicket(); Silence verbose logging; Link to repo Jun 19, 2023
Copy link
Collaborator

@danhunsaker danhunsaker left a comment

Choose a reason for hiding this comment

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

Looks good overall, but it might be nice to make a couple tweaks. See my other comments for details on what I mean.

pve2_api.class.php Outdated Show resolved Hide resolved
pve2_api.class.php Outdated Show resolved Hide resolved
Comment on lines 260 to 272
error_log("Error - Invalid HTTP Response.\n" . var_export($split_headers, true));
throw new PVE2_Exception("PVE2 API: Error - Invalid HTTP Response.\n" . var_export($split_headers, true));
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Throw/return paradox again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if you want everything reverted to error_log. Excepting is helpful as we can expect a proper reply in integrated apps, and relay the error specifics to the end user. So I'm hesitant to revert everything?

Copy link
Collaborator

Choose a reason for hiding this comment

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

All I meant was that once you throw, the return never gets called.

Comment on lines 267 to 280
error_log("\$action_response_array['data'] is empty. Returning false.\n" .
throw new PVE2_Exception("PVE2 API: \$action_response_array['data'] is empty. Returning false.\n" .
var_export($action_response_array['data'], true));
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Throw/return paradox again.

@lsthompson
Copy link
Collaborator Author

lsthompson commented Jun 20, 2023

Looks good overall, but it might be nice to make a couple tweaks. See my other comments for details on what I mean.

Thanks for the review @danhunsaker - how's the updated pull looking now? Appreciate your pointers on all fronts.

I'd like to switch back the reverted-to-error-log section? I'm a fan of passing back the specifics if it's doable.

@lsthompson lsthompson requested a review from danhunsaker June 20, 2023 00:46
lsthompson added a commit that referenced this pull request Dec 22, 2023
Comment out the verbose error logging segment. Plan is to expose this via option as part of Issue #47 and Pull #42
@lsthompson lsthompson self-assigned this May 14, 2024
danhunsaker pushed a commit that referenced this pull request Jun 24, 2024
Comment out the verbose error logging segment. Plan is to expose this via option as part of Issue #47 and Pull #42
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.

2 participants