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

ISSUE-2 add CLI to allow easier automation #37

Merged
merged 6 commits into from
Jan 7, 2019
Merged

ISSUE-2 add CLI to allow easier automation #37

merged 6 commits into from
Jan 7, 2019

Conversation

lewismc
Copy link
Contributor

@lewismc lewismc commented Feb 20, 2018

The presence of the ISSUE-2 branch was breaking out Packagist.org compliance. Instead of maintaining it as a branch, this opens a PR for review by the team.
@eniad FYI thanks for the PR.

Wright, Daine M and others added 3 commits June 21, 2017 14:35
needed to add code to startup.php to get script name.
is this always save and does it return script name?

`elseif ($_SERVER['argv'][0] != '') { // php CLI
    // TODO: check to see if this is safe and always true
    $SETTINGS[PSNG_SCRIPT] = $_SERVER['argv'][0];`
@lewismc
Copy link
Contributor Author

lewismc commented Feb 27, 2018

@eniad I am thinking that some documentation to accompany this PR would be helpful. Naybe we can augment the README wdyt?

@lewismc lewismc added this to the 1.7.0 milestone Feb 27, 2018
sitepod-cli.php Outdated

ob_start (); // Start buffering output

require_once (dirname ( __FILE__ ) . '/cron.php');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why should a CLI import the whole cron.php?

sitepod-cli.php Outdated
// TODO: add verbose text, html, and quiet text flags
$text_only = 1;

ob_start (); // Start buffering output
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest to avoid comments like this.
The code should explain itself.
ob_start() is basic function, we don't need to explain it.
ob_get_clean() and strip_tags() are also basic functions.

sitepod-cli.php Outdated
$results = ob_get_clean (); // Put the buffered output into $results and clear the output buffer

if ($text_only) {
$results = strip_tags ( $results ); // Strip HTML and PHP tags from results
Copy link
Collaborator

Choose a reason for hiding this comment

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

Strip HTML and PHP tags from results
In my opinion strip_tags() is a basic function, we don't need to explain it in a comment.
BTW: $results don't have any php tags, I think.

@sigee
Copy link
Collaborator

sigee commented Mar 1, 2018

@lewismc, @eniad,
What is it exactly?
I mean a CLI is not equal with the CRON script.

@lewismc
Copy link
Contributor Author

lewismc commented Sep 12, 2018

Hi @sigee I went ahead and removed the unnecessary comments. We still import the /cron.php however.
What are you comments on the current PR?
Thanks you and sorry for the late response.

@lewismc
Copy link
Contributor Author

lewismc commented Nov 30, 2018

@sigee ping if you have time to review...

@sigee
Copy link
Collaborator

sigee commented Dec 29, 2018

@lewismc
It seems a bit better, but I still not know why the whole cron.php is needed.
If so than it is OK till a "small" refactor.

@lewismc lewismc merged commit a28a5ad into master Jan 7, 2019
@lewismc lewismc deleted the ISSUE-2 branch January 7, 2019 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants