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

Redirect healthcheck #156

Closed
wants to merge 12 commits into from
Closed

Conversation

polinart
Copy link

  • added 'status' field in 'redirects' table
  • added job to check redirects and update 'status' field
  • added console command to check few table record or all records and update status
  • added observer to handle 'saved' event: check url and update status

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 2.042% when pulling 69ec5aa on polinart:redirect-healthcheck into 1b8b6fb on arbory:1.0.

@coveralls
Copy link

coveralls commented Jul 23, 2019

Coverage Status

Coverage decreased (-0.03%) to 2.038% when pulling cb9648b on polinart:redirect-healthcheck into 1b8b6fb on arbory:1.0.

composer.json Outdated
@@ -32,7 +32,8 @@
"roboc/laravel-glide": "^1.1.0",
"spatie/laravel-sluggable": "^2.1.0",
"maatwebsite/excel": "^3.0.5",
"unisharp/laravel-filemanager": "^1.8.5"
"unisharp/laravel-filemanager": "^1.8.5",
"ext-curl": "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this up, group with other extensions

*/
protected $signature = 'arbory.redirect-health
{ids=[] : The array of IDs from redirects table to check (if not provided then would be selected all redirects table entries)}
{--errors : Show curl request errors}';
Copy link
Contributor

Choose a reason for hiding this comment

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

-v --verbose are standart flags to increase console output. Could we use those?

* @var string
*/
protected $signature = 'arbory.redirect-health
{ids=[] : The array of IDs from redirects table to check (if not provided then would be selected all redirects table entries)}
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we pass "array" via CLI? Please specify that command expects comma separated values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really necessary? Who would ever know ID for specific redirect entry.

*/
public function handle()
{
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't put all your code in Try block, this is bad error suppression

$result = $job->getResult();

if (! empty($result) && count($result->getInvalidUrlList())) {
$this->warn("\nInvalid URLs list:");
Copy link
Contributor

Choose a reason for hiding this comment

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

Use PHP_EOL for new line

$this->error('Redirects healthcheck failed with an exception');
$this->error($e->getMessage());

return 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 2? Magic nu,ber

}

$this->warn("\nInvalid entries: {$result->getInvalidCount()}");
$this->info("Valid entries: {$result->getValidCount()}");
Copy link
Contributor

Choose a reason for hiding this comment

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

Use sprintf or concat for string concatination

@exabyssus
Copy link
Contributor

exabyssus commented Nov 5, 2019

This pull-requests still has multiple issues and was voted - unnecessary for core Arbory. Closing for now.

@exabyssus exabyssus closed this Nov 5, 2019
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.

None yet

3 participants