Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

Fix top scorers for excluded leagues #46

Merged
merged 3 commits into from
Oct 20, 2018
Merged

Fix top scorers for excluded leagues #46

merged 3 commits into from
Oct 20, 2018

Conversation

Visovsiouk
Copy link
Contributor

Pull Request [Fix top scorers for excluded leagues]

Added CustomSoccerApi that extends SoccerApi from the kirill-latish package. The new class calls to CustomTopScorer request in which I have added the aggregatedBySeasonId() method which does not exist in the original package.

  • Bug
  • Feature
  • Minor Change

Does it fix an existing issue? Please tell us which one

Resolves #36

@sebastiaanspeck
Copy link
Owner

Have you tested it with multiple leagues? I think it will also use the topscorersAggregated for other leagues that are not having this issue

@Visovsiouk
Copy link
Contributor Author

@sebastiaanspeck What should be the desired result to test?

@sebastiaanspeck
Copy link
Owner

Well, right now the top scorers are not shown for World Cup.
At this moment this is no issue for Champions League and Europa League, but this will become an issue when those tournaments reach knock-out phase.

These leagues need to use this aggregated but I don't want to put these league-ids hardcoded so you got to find a way to seperate these leagues from the leagues who don't need the aggregated

@sebastiaanspeck
Copy link
Owner

test your code with the next leagues:

  • Pro League -> these one doesn't show any top scorers because of

    if(count($topscorersAggregated) > 0) {
        $topscorers = $topscorersAggregated;
    } else {
        $topscorers = array();
    }
    

    instead it needs to use the $topscorersDefault and only request the topscorersAggregated if the league is World Cup, Champions League and Europa League.

  • Premier League -> this one also uses the $topscorersAggregated, which is incorrect, but it also
    shows all 123 topscorers. I want a top 10.

  • FA Cup (or any other cup) -> this one doesn't need to show any top scorers

  • World Cup -> show the aggregated top scorers

  • Champions League/Europa League -> show the aggregated top scorers, but leave out the top scorers from the qualifications-rounds

@sebastiaanspeck
Copy link
Owner

Use the current master-branch as starting point.

I have a folder soccerapi in the root. If you copy the contents of it to vendor/kirill-latish, you can use

$soccerAPI->topscorers()->setInclude($includeTopscorersAggregated)->aggregatedBySeasonId($league->current_season_id)->aggregatedGoalscorers->data;

@Visovsiouk
Copy link
Contributor Author

I've been able to match all your requirements, except for the last one. There doesn't seem to be an endpoint in the API that separates topscorers per round/stage, only per season.

Repository owner deleted a comment Oct 20, 2018
Repository owner deleted a comment Oct 20, 2018
Repository owner deleted a comment Oct 20, 2018
@sebastiaanspeck
Copy link
Owner

You can get all possible stages for the league. View the docs for the league-endpoint and you can exclude the stages we don’t want using the stage-ids

@sebastiaanspeck sebastiaanspeck merged commit a827bed into sebastiaanspeck:master Oct 20, 2018
Repository owner deleted a comment Oct 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix top scorers for excluded leagues
2 participants