Change recommendations back to html parsing. Remove year and votes

tboothman / imdbphp

PHP library for retrieving film and tv information from IMDb

253 stars 84 forks source link

Change recommendations back to html parsing. Remove year and votes #232

Closed duck7000 closed 3 years ago

duck7000 commented 3 years ago

Added img url in xpath fallback part.
Convert runtime from seconds to minutes in xmlNextJson part. it is easier to read/work with.

tboothman commented 3 years ago

I don't like this fallback business. Either the first one always works and the fallback is pointless or the first one doesn't always work and you might randomly get different fields in yours results. If i delete the xpath section the tests still pass ... what does that mean? Why did you add the img field if that section of code does nothing?

I agree seconds is nonsense and the runtimes are specified in minutes on the edit page so it makes complete sense to use that

duck7000 commented 3 years ago

@tboothman

Thanks for your comment!

Well i assumed that the fallback part actually works and does something and seen the fact that you merged this although this was part of a large amount of commits. That's why i added the img url and because it is also added to the json part. But i agree that it is kind of pointless to have the fallback part now i think about it and read your comment. I thought about this, especially about the fact that the output array is not consistent the same as the data differs

And for the record i didn't add the fallback part, just add something to it.

So what to do next? Remove the fallback part at all? I did that in my tests on my own server and it always works so the fallback part can safely be deleted i guess?

duck7000 commented 3 years ago

I removed the fallback part completely.

duck7000 commented 3 years ago

The output of xmlNextJson is not consistent, first time the results are completely different, second time and beyond the output keeps consistent and is the same as shown on the actual webpage. The fallback method is more reliable as it gets data from what is shown on the actual page. I revert it back to this method. The downside is that there is less info available.

tboothman commented 3 years ago

Tests are failing because the year is missing from the results

duck7000 commented 3 years ago

@tboothman

Thanks for commenting! Sure it fails, that info is not available so the test needs adjustments. honestly i haven't look at it, do i have to fix the test as well?

tboothman commented 3 years ago

The tests are to ensure that the code continues to do what it did in the past. The year not being there showed that this was not the case. It's fine, but it just has to be noted in the release as it's not backwards compatible. I'm not that bothered in this instance as I doubt that many people use this method and if they do it's likely they only use the id field. The options are:

to keep using that json that people don't understand and get unreliable results .. which isn't really acceptable
Remove the field and go back to xpath which is reliable
Someone work out properly how that json works and make it be reliable

I'm fine with this outcome

duck7000 commented 3 years ago

@tboothman Thanks for your comment and patients Yeah you are right about the year, but in this case the year is not their anymore so not backwards compatible is possible . But indeed it needs to be noted. Thanks for merging!