duck7000 / imdbGraphQLPHP

7 stars 0 forks source link

Logger needed? #48

Closed duck7000 closed 5 months ago

duck7000 commented 6 months ago

Is the logger still needed?

if so in what form? Does it have to log to file or to screen? or both What needs to be logged? there is no point to log all methods been successfully called i guess GraphQL api call can be logged if anything goes wrong for example

GeorgeFive commented 6 months ago

Honestly, this works really well, I rarely have problems with it.... but when something does go wrong (especially in automated, behind the scenes scanning), it would be nice.

Example, sometimes pages get completely removed from imdb, not just a redirect (mainly with fake credits getting deleted). So I have the old data cached, I try to scan the (now 404) imdb id for this person, and you get a 404 error. I haven't encountered this in a while, but it used to be a hard exit on the script. So, I'm trying to scan 500 names, but we get the hard exit, so nothing happens after that... then it happens again, and again, until I notice that nothing is being scanned for a few days and fix that manually.

This one wouldn't affect me, as I use the image size=medium setting, but the large images would crash php. Maybe a log notification if that happens? Because my script will delete the existing image, then try to get the new one.... but it crashed.... so I now have a broken image link.

I guess these are edge cases that will rarely happen, but it would be nice to see them when they do happen.

duck7000 commented 5 months ago

Mm i have to think about this

How would i have to log this is the question, by removed you mean that the imdbid is deleted? so the page doesn't exists anymore. imdbphp6 will crash/output nothing as far as i can tell? This can be problem so we have to think about this.

Okay so we need to log the large images but when do i know that that image is too big and crash php? I need to know web server memory limit and the image size in ram memory

A tip, check the new image before deleting the old one. How to do that is a big question because how/when do you know if php is going to crash or not

Maybe removing the full/large image url will help? Those images are known to be very big (2k, 4k and even i have seen 8k images) so this is impossible to work with anyway

duck7000 commented 5 months ago

I think the max image size is around 1000 pixels, anything above would be considered too large to handle i guess?

So we might get the image width and height, if imageSize is set to "large" (in photo method) and width or height exceeds 1000 pixels log it to file? Or return nothing/a smaller version etc.

For screen presentation of those images 1000 pixels is already very large to begin with. In my program i only use the thumb variants so why do you needs such big images is a question?

GeorgeFive commented 5 months ago

Personally, I use the medium setting and that does what I need. I'm just thinking if someone uses large and encounters this issue, it could be a problem for them? Maybe setting max to 1000 in large would help.

This is the image that prompted this....

https://www.imdb.com/name/nm9526605/

duck7000 commented 5 months ago

Did you check how big this image in full res would be? I did... 14712 x 22067 yes that is correct! This would kill any webserver!

Setting large to max 1000 pixels would make medium setting no longer necessary? This would revert all changes back to thumb true or false (true thumb, false large with max 1000 pixels)

GeorgeFive commented 5 months ago

That could work. Yeah, I saw the resolution on that image, and I think that's the second one I've seen like that. Personally, I'm fine with medium as is, I'm just thinking if someone uses large for their own reasons, they might hit trouble with an image like that.

I'm ok with either direction you want to take on this, but I just wonder if someone else may want the large image option?

duck7000 commented 5 months ago

well 2 options as i see it

Or we remove the full large image (as in some cases it is unusable big anyway) Medium will remain (and added to Title class photo as well)

Or the full large image will be restricted to 1000 pixels (medium will be removed)

I tend to the last one?

duck7000 commented 5 months ago

i changed the photo method in Name class parameter is back to $thumb true default false large image max 1000 pixels

Title class photo has the same outcome as Name photo

next is episodes, i will do the same, remove full image, large is max 1000 pixels This is done.

GeorgeFive commented 5 months ago

Looks good!

duck7000 commented 5 months ago

@GeorgeFive

Logger is back added, only for imdb api call at the moment

test it out if you have the time (especially with visualEffects() and specialEffects() and see if that gives a error of some kind

GeorgeFive commented 5 months ago

PHP Fatal error: require_once(): Failed opening required '/imdb6/src/Imdb/Logger.php'

Don't see that file listed on here...?

duck7000 commented 5 months ago

Oops, i forgot to upload that file hah It is now corrected in latest git

duck7000 commented 5 months ago

closing this one as the logger works