tboothman / imdbphp

PHP library for retrieving film and tv information from IMDb
253 stars 84 forks source link

Add a parameter to cast() to get only the cast list from the title page #142

Closed sebastian-king closed 6 years ago

sebastian-king commented 6 years ago

This just adds a $short parameter to cast() to get only the actors and role listed on the title page.

I found this necessary because when doing a search, it would return a number of titles and if I wanted to get any of the cast it would have to execute another request to /fullcredits, dozens of times. My searches ended up taking in excess of two minutes to complete with all of those extra requests and parsing and caching. If I just get the main cast listed on the title page, my search can display the important actors and complete in under a second.

PS: I also removed the description for the $clean_ws parameter for cast() as that parameter no longer exists.

jreklund commented 6 years ago

Hi, thanks for your code submission. Looks great to have a shorter option. I'm afraid you put the empty page check in the wrong place.

Should look like this.

    if ($short) {
      $page = $this->getPage("Title");
    } else {
      $page = $this->getPage("Credits");
    }

    if (empty($page)) {
      return array(); // no such page
    }

So just make a push to sebastian-king:master and it will be automatically added here. After that I will just test the code and then merge it.

Do you know how to make tests? We need one for cast(false) in case the change the style on the Title page but not on Credits.

sebastian-king commented 6 years ago

Ah, I had assumed that $this->getPage("Title") would always be defined, and tried to reduce the if statement. But I will move the statement outside as a double check.

I do know how to make tests, do you want me to make an equivalent testCast_short test for all of the current testCast cases?

Also, I had to replace extends PHPUnit_Framework_TestCase with extends PHPUnit\Framework\TestCase to get it to work with my version of PHPUnit. I'm not sure if that's something that needs to be updated. I'm using PHPUnit 6.5.5 on PHP 7.2.10.

jreklund commented 6 years ago

It just a precaution just in case there are something wrong while fetching it.

One is good, just copy testCast_film_with_role_link. It's just so we know that they haven't changed the layout/id on the Title page.

Just change it back before making a commit. So it will be correct on our version.

sebastian-king commented 6 years ago

I committed those changes and ran the test and it checked out. I'm not sure why the newline at the end of the file went away, my text editor probably purged it.

tboothman commented 6 years ago

I like it. The only time I've ever got the cast I limited it to 10, i doubt many people want the big list that requires an alternate page to be fetched