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

Use GraphQL to fetch paginated keywords #310

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jetrosuni
Copy link
Contributor

Fixes #308

Replaces legacy scraping logic with working GraphQL query.

@jetrosuni
Copy link
Contributor Author

Note: The failing tests are not related to this PR. testKeywords_all works.

@duck7000
Copy link
Contributor

I came up with this solution, i don't really know which is better? i think that graphQlGetAll() not intended is to use it like you did but i can be wrong i guess?

    public function keyword()
    {
        if (empty($this->all_keywords)) {
            $query = <<<EOF
query Keywords(\$id: ID!) {
  title(id: \$id) {
    keywords(first: 9999) {
      edges {
        node {
          keyword {
            text {
              text
            }
          }
        }
      }
    }
  }
}
EOF;

            $data = $this->graphql->query($query, "Keywords", ["id" => "tt$this->imdbID"]);
            foreach ($data->title->keywords->edges as $edge) {
                $this->all_keywords[] = $edge->node->keyword->text->text;
            }
        }
        return $this->all_keywords;
    }

@jetrosuni
Copy link
Contributor Author

Yeah, I guess most movies on IMDb have less than 9999 keywords, so your proposed solution might well be enough :) With graphQlGetAll()the logic will continue to fetch more keywords if the limit of 9999 results would ever get hit. I'm okay with either solution – @tboothman can make the call :)

@duck7000
Copy link
Contributor

I have hit a 250 items limit from imdb GraphQL API on cast. I changed my method to use the paginated function to get all cast members.
So your solution might be better as it gets all pages, although i did not hit this limit with keywords so imdb does not have that 250 item limit on all methods i guess.

So far only cast and episodes seems to have that limit

@jetrosuni
Copy link
Contributor Author

Any issues with this PR, or can it be merged? Happy to help and fix if issues are pointed out!

@duck7000
Copy link
Contributor

Well i don't think that any of the maintainers take your PR serious as @jreklund just released a new version with a fixed keywords method but not with GraphQL, unbelievable!
And i even did offer him to use my work to fix imdbphp..

@jetrosuni jetrosuni force-pushed the use-graphql-to-fetch-keywords-all branch 4 times, most recently from afcb64d to 1c12a33 Compare February 25, 2024 14:11
@jetrosuni
Copy link
Contributor Author

I'm okay with the maintainers doing the best they see fit and just happy that the library is not completely abandoned. I've now based this PR on top of the latest release. Hopefully one day it can be merged :)

@jetrosuni jetrosuni force-pushed the use-graphql-to-fetch-keywords-all branch from 1c12a33 to 8717692 Compare February 25, 2024 14:19
@jcvignoli
Copy link

Well i don't think that any of the maintainers take your PR serious as @jreklund just released a new version with a fixed keywords method but not with GraphQL, unbelievable! And i even did offer him to use my work to fix imdbphp..

Please calm down and stop complaining. You started your fork, which is fine, so please show some respect to the orginal makers of IMDbPHP. Thanks.

@duck7000
Copy link
Contributor

I'm not complaining at all, i just think that those fixes don't fix anything, and what does get fixed (in this PR) the right way is ignored. I almost feel sorry for the maker of this PR as his work is fine but is ignored. I mean it is al here why don't use it?
This has nothing to do with respect, i do respect jreklund! I just don't understand it.

But okay fine, good luck with it.

@jetrosuni jetrosuni force-pushed the use-graphql-to-fetch-keywords-all branch from 8717692 to 3da6df7 Compare March 2, 2024 01:34
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.

keywords_all() no longer returns any data?
3 participants