duck7000 / imdbGraphQLPHP

5 stars 0 forks source link

Anyone seen more methods with 250 items limit? #27

Closed duck7000 closed 2 months ago

duck7000 commented 9 months ago

The cast method has had a 250 items limit, i fixed that but i'm wondering if there are more methods with this limit?

I'm thinking about keywords for example, however this still gives more than 250 items while keywords are paginated at imdb? So that limit is not on all methods, so far only cast and episode seems to have it

GeorgeFive commented 9 months ago

I would say that, theoretically, a movie could have more than 250 producers / directors / writers, though I haven't actually seen this. The closest I've seen is:

https://www.imdb.com/title/tt2395133/ - 186 producers credited https://www.imdb.com/title/tt4942694/ - 66 directors https://www.imdb.com/title/tt4490348/ - 43 writers https://www.imdb.com/title/tt0013442/ - 17 composers

Note that this was only sampling movies I have listed in my own project, which is NOT all-encompassing by any means!

duck7000 commented 9 months ago

Thanks for testing!

The biggest issue is whether or not imdb has a limit on those GraphQL fields, if not there is no problem. Problem is that i can't know for sure as there is no documentation about this

For now if anyone finds a method that does cap on 250 while there are for sure more items let me know. I will check if that method has that limit or not. That's all we can do.

duck7000 commented 4 months ago

Closing this one as this does not seem the case for the other methods

If there still are re open this

GeorgeFive commented 3 months ago

Episodes seems to be capping out at 250 episodes per season...

https://www.imdb.com/title/tt0121220/episodes/

EDIT - Forgot about the huge discussion a while back about this. Not sure if this is any easier to fix now, but since I'm sure you got the email notification, I'll leave this be...

duck7000 commented 3 months ago

Yes i remember that discussion haha But you are right, there is a limit but i left it to rest because i didn't (and still find it idiotic) that there possibly could be more then 250 episodes in one season (that seems technically impossible to do) But in this case imdb chose to put all episodes in one season despite that there are multiple years involved. So this will always be difficult but as they present it to us it is correct

So this is a edge case but i will try to fix this (it worked with cast so this should work too) thanks for letting me know though!

duck7000 commented 3 months ago

I got it working to collect all episodes!

While working on this problem i looked again at the code and there is room for improvement.

The airdate: I get the day month and year from imdb and fabricate from that a text string, which is stupid because imdb provide the airdate as text string... So the question is do we want the airdate as text (like it is know but i make it simpler/better in that case) or return the airdate as separate array items , or both?

And the image is now fixed returned as 224x 126 pixels, so do we want to add parameters to get any image size you want? (it will be a cropped image just like imdb does on their site) i figured all this stuff out how they crop the images (that was a massive undertaking) so we might as well give the user the option to choose? I made it fixed as i want those images at that specific size but somebody else might need a different size

GeorgeFive commented 3 months ago

For airdate, I would leave it as it is now, or at least leave the existing date array. This way, the user can decide how they want to format the date (I prefer October 14, 2002, you might want 2002-10-14, others may want 2002 Oct 14, etc). I'm not sure how imdb handles region differences in date displays....?

I'm good with whatever on the image sizes, as long as we can continue to get the original image (default?). I already store the images on my side and do my own cropping as needed, so I think I'm good on that end, personally.

duck7000 commented 3 months ago

Mm airdate can be messy

The plainText from GraphQL is like this: October 31, 2015 (which you prefer) I prefer: 31 Oct. 2015 (as it is shorter) (this is like it is now)

So what is wise to do? I will add the separate date parts for sure.

Right now the image is always returned as cropped 224x126 so there is no full image. Do you want a parameter for the image like full or cropped? Or shall i add the full image as well so both are always available? Personally i think a parameter would be better as we use that in other methods too

GeorgeFive commented 3 months ago

Ok, for dates, I was getting mixed up with the person class... we return year, month, and day as separate values, but for airdate, we just get the one date. I forgot that I was doing php date formatting on $episode['airdate'], so yeah, I would just leave that value alone and maybe add airdatetext?

Image is another one, I forgot that I did some hacking on it (maybe I should look at my code before replying here). I do prefer the full image, and what I've been doing is....

$image = pregreplace("/(..+?_.jpg)/i", ".jpg", $episode['image_url']);

...and then saving that image to my server and doing my own cropping. A parameter would probably be easier, definitely.

duck7000 commented 3 months ago

Ah i understand

I want to adjust this just to prevent users from needing to "hacking" the output. So the output should be usable for all users, if they what something specific (like a image size not provided) they can use the full image url to do so.

I can make airdate like Name class if you want? (that is what i meant output to array(day, month, year) That would be more solid then re formatting the date string i guess? The date as text would/can be as provided by imdb api (like you want, i can live with that too)

GeorgeFive commented 3 months ago

I'm good with whatever on the date formatting for airdate. I'm personally fine with how it is right now, I don't really need the other options, but it's a quick fix on my end if you think it should be updated.

Definitely in agreement on the image parameter, I completely forgot that I had hacked that little fix into my code and thought that was just standard, haha. So we'll just call it with $size="medium" or something like in person?

duck7000 commented 3 months ago

About the image i suggest to make a parameter that indicates thumb or full image. Medium in Name class is just added for your need (i doubt if somebody else uses this), in this case i think it is best to use thumb or full so everyone can use what they want? That would be the cleanest solution.

From a clean data perspective airdate should be provided as array(day, month, year) but that does mean we both need to adjust our programs. So i suggest that i add airdate array and keep the existing airdate as text so other users would have the choise either use the date parts or the provided airdate as text

duck7000 commented 3 months ago

I have added all changes to episodes and pushed a new release

Release notes and wiki provide all info about the changes

@GeorgeFive Give it a try if you have the time

GeorgeFive commented 3 months ago

Remember, we added medium to compensate for #21 ... I don't have any examples off hand, but would it be possible for this issue to pop up again if we find a really large episode image? That's why I stuck with medium, it was big enough for my needs but avoided the insanely huge images that kill PHP.

Otherwise, no issues, all looks good!

duck7000 commented 3 months ago

I forgot about #21 at all..

So yes in theory the images can be very big, that is also why i suggested to add image dimension parameters. That way you can set what exact size you want but yes it will be a cropped image.

I remember that you don't want a cropped image so i have to think of another way to tackle this. So yes maybe is adding medium a solution, i'll think about it.

GeorgeFive commented 3 months ago

I think medium might be the way to go... as I'm sure you know, they use both horizontal and vertical images, which gives two very different cases for cropping....

Default settings, this.... MV5BM2Q0MTkyMmQtMGJmOS00M2E5LTkxZjktZjlkZjAzYjk2MTA4XkEyXkFqcGdeQXVyMTQxNzMzNDI@ _V1_SY931_ becomes MV5BM2Q0MTkyMmQtMGJmOS00M2E5LTkxZjktZjlkZjAzYjk2MTA4XkEyXkFqcGdeQXVyMTQxNzMzNDI@ _V1_QL75_UX500_CR0,227,500,281_ ...which just looks silly, haha.

I'd rather just get the exact image proportions that they use, no cropping, but the image doesn't need to be 25mb+ (in theory, I haven't actually seen it yet...)

duck7000 commented 3 months ago

Well.. yes i see what you mean but this cropping is intentional to become a landscape image cropped from the center (as i use it like this)

But yes other users, like you, would want something else so i will add a medium setting

duck7000 commented 3 months ago

I changed the image sizes (available as latest commit) small, large and medium is now available just like in Name class Info is adjusted in wiki

I did changed the array element name img_url to imgUrl as i use camelCase everywhere

@GeorgeFive check it out!

GeorgeFive commented 3 months ago

All is working well, awesome!

duck7000 commented 3 months ago

Thanks

Glad we tackled jet another problem haha

duck7000 commented 3 months ago

@GeorgeFive The issue with image urls may also be a problem with cast, recommendations and mainphoto and photo images, do you want to change that too? They all return cropped images for my specific use though

GeorgeFive commented 3 months ago

I do not use any of those images, so I'm a bit neutral on those. Might make sense to make everything similar, but I won't lose sleep if we don't update them either...

duck7000 commented 3 months ago

Okay since i'm the only one that uses those images i leave it like it is, thanks

duck7000 commented 2 months ago

re open this one as there are probably more methods that needs conversion to GraphQLGetAll as they might have more then 250 items

keywords companyCredits crazyCredits trivia goof quote award (this is a big one and could also need stripSpaces as the query is large) filminglocation releaseDates Also known as connection alternateVersion soundtrack mpaa faq

All above methods are now converted with the exception of also known as. For this one i have to create a special graphQlgetAll method and i doubt that it is worth it, but i might do it later on.

duck7000 commented 2 months ago

@GeorgeFive CompanyCredits (prodCompany, distCompany, otherCompany and specialCompany) are now converted to GraphQLGetAll so those method can fetch more than 250 items (if available)

I added stripSpaces to graphQlGetAllEpisodes method to avoid future problems with hosters limits. I will do this for all methods that are converted to graphQlGetAll. For single value methods, or methods with low output this is not necessary

All title methods are now converted except also known as (see above)

duck7000 commented 2 months ago

Also known as is not possible to convert to graphQlGetAll and i doubt it is necessary at all. For now i leave it like it is now, if anyone finds a title that do have a problem provide me with a example and i will look in it.

Edit: it is possible to convert aka as well, i changed original title to use the originalTitle() method instead of extra query. With this change it is possible. And i fixed a bug with comments..

duck7000 commented 2 months ago

Also known as is now converted to graphQlGetAll so all that needed this are done.