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

Check beatmap cover images on qualification #2692

Merged
merged 3 commits into from Mar 16, 2018

Conversation

4 participants
@nekodex
Collaborator

nekodex commented Mar 14, 2018

...and queue a regeneration if any are missing

break;
}
}
} catch (\Exception $e) {

This comment has been minimized.

@nanaya

nanaya Mar 14, 2018

Collaborator

what is this catching?

try {
foreach ($this->allCoverURLs() as $size => $url) {
$ch = curl_init($url);
curl_setopt($ch, CURLOPT_HEADER, true);

This comment has been minimized.

@nanaya

nanaya Mar 14, 2018

Collaborator

there's this curl_setopt_array.

{
// trigger a cover regeneration if any cover images are missing
if (!$this->beatmapset->allCoverImagesPresent()) {
$job = (new RegenerateBeatmapsetCover($this->beatmapset))->onQueue('beatmap_high');

This comment has been minimized.

@nanaya

nanaya Mar 14, 2018

Collaborator

Apparently there's Dispatchable trait which may simplify things.

This comment has been minimized.

@nekodex

nekodex Mar 15, 2018

Collaborator

Yeah I don't think it really helps... if anything, I'd argue it makes it less readable not more readable?

$job = (new RegenerateBeatmapsetCover($this->beatmapset))->onQueue('beatmap_high');
dispatch($job);

vs

(new RegenerateBeatmapsetCover($this->beatmapset))->dispatch()->onQueue('beatmap_high');

This comment has been minimized.

@notbakaneko

notbakaneko Mar 15, 2018

Contributor

I don't think Dispatchable helps when the job is a different class.

This comment has been minimized.

@nanaya

nanaya Mar 15, 2018

Collaborator

more like

RegenerateBeatmapsetCover::dispatch($this->beatmapset))->onQueue('beatmap_high');

This comment has been minimized.

@nekodex

nekodex Mar 15, 2018

Collaborator

My readability complaint remains, dispatch()->onQueue() makes no sense to me, I expect dispatch to... dispatch, not to be chained onto to enqueue elsewhere

$allGood = true;
try {
foreach ($this->allCoverURLs() as $size => $url) {
$ch = curl_init($url);

This comment has been minimized.

@nanaya

nanaya Mar 14, 2018

Collaborator

better off as helper function?

@@ -830,6 +831,32 @@ public function regenerateCovers(array $sizesToRegenerate = null)
$this->update(['cover_updated_at' => $this->freshTimestamp()]);
}
public function allCoverImagesPresent()
{
$allGood = true;

This comment has been minimized.

@nanaya

nanaya Mar 14, 2018

Collaborator

a bit shorter...

... function ...()
{
    foreach (... as ...) {
        if (!url_check(...)) {
            return false;
        }
    }

    return true;
}

?

<?php
/**
* Copyright 2015-2017 ppy Pty. Ltd.

This comment has been minimized.

@nanaya

nanaya Mar 14, 2018

Collaborator

2018 =D

nekodex added some commits Mar 15, 2018

@peppy peppy merged commit ff53667 into ppy:master Mar 16, 2018

2 checks passed

continuous-integration/styleci/pr The StyleCI analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nekodex nekodex deleted the nekodex:check-beatmap-covers-on-qualify branch Mar 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment