duck7000 / imdbGraphQLPHP

6 stars 0 forks source link

Person class #2

Closed duck7000 closed 10 months ago

duck7000 commented 1 year ago

The plan is to add back person class in this version.

Questions: @GeorgeFive Does anyone need personSearch class? What methods should be minimal included in person class?

duck7000 commented 1 year ago

i converted the personSearch class and uploaded it so that is working. Although i'm not sure how many results should be returned? for now it is set to 10

GeorgeFive commented 1 year ago

I don't think personSearch is totally important... I get all of the cast into from title lookups, and then I get cast information for each person by IMDb id. I'm sure some other people may disagree though, so in that case, I'd definitely say 10-20 is sufficient.

duck7000 commented 12 months ago

I'm working on person class, about 2/3 done sofar. Most parts are not that difficult so i converted a lot

duck7000 commented 12 months ago

Person class is done!

please check if it is working right and suggestions for improvements are always welcome. height is still available (wasn't that hard to convert)

GeorgeFive commented 12 months ago

Sounds awesome! I am currently on a crazy work schedule until at least Friday, so I don't have much time to play with it. I'll see what I can do between now and then, but I'll definitely give it a full test when I can.

One minor request... I'm not terribly used to github, so I probably screwed up, but I tried to add season number to title.php and I'm not seeing it go through on here. Would it be possible to throw it in around line 1301 so I don't have to keep manually adding it back?

'season' => $seasonFilter,

Thanks!

duck7000 commented 12 months ago

Consider it done Added season to episode array (personally i don't use that but others like you do)

$seasonFilter is not season, it is used to add a filter to graphQL query

jcvignoli commented 12 months ago

@duck7000 Started using it, to many functions from tboothman version do not work anymore. Great work and thanks!

duck7000 commented 12 months ago

I'm glad it worked for you.

Others are not that happy, lot of methods missing and they stick to tboothman version and that is fine of course but yes many methods are not working anymore

duck7000 commented 12 months ago

Let me know if there are improvements necessary

jcvignoli commented 12 months ago

This is the life of open source projects: they sometimes just disappear! I'll let you know should I need new functions, thanks!

Thomasdouscha commented 11 months ago

Sometimes return of data from Person Class has ' like html code. It is very big issue for data processing. Please convert html charcode to utf8 text.

duck7000 commented 11 months ago

@Thomasdouscha What method are you using? There is no html outputted anywhere as far as i know so i don't understand what is going on?

Thomasdouscha commented 11 months ago

Sometimes person class returned data has included "'", "'"some html code. bio(), born() died() methods has this issue. Please check your graphql code about it. For example try irish people as you know their surname has included " ' " and returned it html code. It should not be like this.

GeorgeFive commented 11 months ago

I don't use bio so I can't comment on that, but imdbphp has always returned html encoding on certain characters on my end (name and born/died locations included). I made my own function to update all of these if you want it...?

duck7000 commented 11 months ago

Well according to this page: https://developer.imdb.com/documentation/bulk-data-documentation/key-concepts/ Imdb json output is utf8 encoded so maybe we need to decode_utf8? The data returned is always like this from GraphQL, in person class there is never any output in html so that can't be a problem.

I haven't seen any problems jet and tboothmans version also doesn't any decoding in graphql either

@Thomasdouscha those &#x27 are special characters, not utf8

I will look in to this

duck7000 commented 11 months ago

@Thomasdouscha Can you give me some examples where this is a problem?

Do you have any problems with title class as well?

GeorgeFive commented 11 months ago

I've tried tackling this problem before, and I ended up giving up on it. Names can have 'apostrophes' and/or "quotes", so simply converting them in the function will lead to php errors when you handle the data. So, the user needs to encode it anyway in order to work with it in their own code, but then you run into the problem of decoding and then reencoding it and corrupting data (I had a hell of a time with, ie, Swedish and German special characters). addslashes is always an option, but that brought up other issues later. In the end, I found it best to leave the characters alone and then fix them via cron as needed.

An example - https://www.imdb.com/name/nm5079930/ - returns Meredith O'Leary

duck7000 commented 11 months ago

if i test @GeorgeFive example i get this in the browser (Firefox) string(16) "Meredith O'Leary"

I just call person class and the name function, nothing else

$movie = new \Imdb\Person("5079930");
$results = $movie->name();
var_dump($results);

So i honestly don't know why this is different at your ends?

will you test if htmlspecialchars_decode($str); makes any difference?

GeorgeFive commented 11 months ago

If you view source on the imdb page, most instances of her name are encoded there... may be your browser decoding it automatically? Looking at my svn logs, I came up with a solution for this back in 2019, so it's definitely not a new issue.... just something I work around.

duck7000 commented 11 months ago

Remember that the data i use comes from GraphQL endpoint, not from the html. So i don't exactly know how that data is presented other than that i know it is utf8 encoded.

@GeorgeFive @Thomasdouscha Can you test if the output data from graphql makes any difference if you change line 55 to this in graphql.php return htmlspecialchars_decode(json_decode($request->getResponseBody())->data);

Or try this on the same line: return utf8_decode(json_decode($request->getResponseBody())->data); i don't exactly know if this will work though, worth a try

Or you can try add this in one of the methods you use to see if that makes any differernce

Thomasdouscha commented 11 months ago

@Thomasdouscha Can you give me some examples where this is a problem?

Do you have any problems with title class as well?

it was too many but i fixed by php code in controller func and i used str_replace. Beleive me there are that kind of issues as i wrote here

Thomasdouscha commented 11 months ago

@GeorgeFive @Thomasdouscha Can you test if the output data from graphql makes any difference if you change line 55 to this in graphql.php return htmlspecialchars_decode(json_decode($request->getResponseBody())->data);

Or try this on the same line: return utf_decode(json_decode($request->getResponseBody())->data); i don't exactly know if this will work though, worth a try

Or you can try add this in one of the methods you use to see if that makes any differernce

tonight i will test. I can say as quess this is better : return utf_decode(json_decode($request->getResponseBody())->data); Because in imdb tehere are many names from different languages.

duck7000 commented 11 months ago

My suggestions to try don't work, sorry

The only thing left to try is to make a helper function to convert output strings in all methods, but it is a lot of work. Or maybe i can make something to convert the data from graphql but i'm not sure how And utf8_decode is deprecated so that is not wise to use anyway

GeorgeFive commented 11 months ago

character_replacements.zip

This is how I do it.... this contains my mysql table with all of the characters I've found that need to be replaced (99 of them). Then I have a cron that uses this data to replace everything that needs to be replaced...

        $query_characters = "SELECT * FROM character_replacements";
        $res_characters = @mysqli_query($link,$query_characters) or die ("characters query fails :".mysqli_error());

        while ($row = mysqli_fetch_assoc($res_characters)) {
            $res = mysqli_query($link,"UPDATE people SET name = replace(name,\"".$row[find]."\",\"".$row[replacewith]."\")") or die(mysqli_error());
            $res = mysqli_query($link,"UPDATE people SET born_location = replace(born_location,\"".$row[find]."\",\"".$row[replacewith]."\")") or die(mysqli_error());
            $res = mysqli_query($link,"UPDATE people SET died_location = replace(died_location,\"".$row[find]."\",\"".$row[replacewith]."\")") or die(mysqli_error());
            $res = mysqli_query($link,"UPDATE people_akas SET name = replace(name,\"".$row[find]."\",\"".$row[replacewith]."\")") or die(mysqli_error());
            $res = mysqli_query($link,"UPDATE movies_castbymovie SET role = replace(role,\"".$row[find]."\",\"".$row[replacewith]."\")") or die(mysqli_error());
            $res = mysqli_query($link,"UPDATE movies_castbymovie SET role_alias = replace(role_alias,\"".$row[find]."\",\"".$row[replacewith]."\")") or die(mysqli_error());
            $res = mysqli_query($link,"UPDATE movies_castbyepisode SET role = replace(role,\"".$row[find]."\",\"".$row[replacewith]."\")") or die(mysqli_error());
            $res = mysqli_query($link,"UPDATE movies_castbyepisode SET role_alias = replace(role_alias,\"".$row[find]."\",\"".$row[replacewith]."\")") or die(mysqli_error());
            $res = mysqli_query($link,"UPDATE movies_episodes SET episode = replace(episode,\"".$row[find]."\",\"".$row[replacewith]."\")") or die(mysqli_error());
        }
GeorgeFive commented 11 months ago

Could it be done easier, sure. But every time I thought I found an easy solution, a problem would pop up that affects some other set of characters. In the end, this was the best, surefire, 100% working solution I could come up with.

duck7000 commented 11 months ago

I honestly don't know how i can help you guys with this problem.

Maybe this is relevant: I use utf8 encoding in my program like this: // UTF8 header. header('Content-type: text/html; charset=utf-8'); Maybe you use something different? hence there is a problem at your end but not mine? If so then you will have to sort the encoding at your end i guess.

And if i view the data directly from graphql api in chrome this looks okay to me (no html entities)

Nevertheless character encoding can be a real nightmare for sure

duck7000 commented 11 months ago

I'm getting back to your encoding problems.

You use older php versions, right? There seems to be a long standing bug in php <8.1 according to Tboothman in this thread: https://github.com/tboothman/imdbphp/issues/264 He has a solution to use htmlspecialchars_decode()

I'm using php 8.1 so it apparently does not effect me i guess, and it only involves single quotes not decoded

Thomasdouscha commented 11 months ago

php ver 7.4 is mine.

Okay, I understand. However, my advice to you is that when creating code libraries, the focus should be on maximizing the number of users who can benefit from it. Still, at least half of the users have PHP 5.6-based applications, while 30% use PHP 7.4, and very few are using PHP 8. I didn't mention this as my problem, it's just a suggestion. Thank you for checking the link you provided.

duck7000 commented 11 months ago

I disagree that my focus should be on maximizing the number of users with a unsupported php version. I did made some changes so this library works on older php versions but my focus lies on supported php versions.

I don't understand why you keep using older unsupported php versions? Too hard to upgrade your code i presume? Or too much work? The program i use in its original state was build on php 4 so i did upgraded all of the code eventually to php 8.1. And yes it was a lot of hard work but worth it.

Thomasdouscha commented 11 months ago

I don't understand why you keep using older unsupported php versions?

That words show me why you disagree. Have you got any complex apllication which is running on web platform? 100.000 lines code and too many vendor application and framework. Can you imagine how is it so hard revising the platform. It takes time and money not easy. And at last i wiil do it. In my oppinion for vendor application what you are doing i can not say it is true. But it is your code and up to you. i respect it. So i hppe tboothman revise their code.

duck7000 commented 11 months ago

Well i do understand that it is hard work and yes if you have a application with 100000 lines of code that it is difficult to do. That is why i added/changed my source code so it would work on older php versions. That does not necessarily mean i agree with your decision to keep on a long eol version, so yes that is up to you

I do agree that if i make a library it should benefit many users but i'm not sure that includes unsupported php versions

After all you asked me, and i did listen and changed/added my code, now you hope that tboothman will fix his version so you can go back? Okay that is also your decision

My code is working on php 5.6 to 8.1 so that can't be a issue

Thomasdouscha commented 11 months ago

Sometimes, it seems to you like I'm just talking about my personal issues, but what I say isn't just about me. We have a huge community with over a hundred thousand members and more than ten thousand active users. Every day, we get traffic from non-members, three times bigger the number of our members. I represent this community, and my concerns are not just about myself but about the whole community.

When I make suggestions, I'm thinking about others benefit, not just mine. I can see that you have a strong sense of sensitivity and empathy, and believe me i am not different from you. Otherwise, I wouldn't have been covering all the expenses of an organization without any income for years. I care about the people we serve, even if I don't know them personally. I plan to take this social project to more advanced versions within the next year.

it's a good practice to keep imdbphp6, which you coded, compatible with the 5.6-8.1 range. Sounds good. Thanks

duck7000 commented 10 months ago

So the person class is working right now?

If so we are getting closer to a release version?

Thomasdouscha commented 10 months ago

yes and yes

duck7000 commented 10 months ago

Thanks for letting me know!

This can be closed now i guess