Return JSON-encoded results from `stats.php` #1345

chrisrosset · 2023-07-15T02:45:00Z

chrisrosset · 2023-07-15T03:50:29Z

@reedy The locale-dependent number formatting changes conflict with my changes. I've resolved the conflicts with -X ours for now. Number formatting has to be re-done on this branch though to match the new expected behavior.

reedy · 2023-07-15T12:47:10Z

@reedy The locale-dependent number formatting changes conflict with my changes. I've resolved the conflicts with -X ours for now. Number formatting has to be re-done on this branch though to match the new expected behavior.

Arguably most of the formatting should probably be done in the frontend, but the current version is/was such a mix...

GitHub doesn't think they are resolved and there are currently conflicts...

reedy · 2023-07-15T13:16:09Z

It seems to pull onto master fine...

chrisrosset · 2023-07-15T13:50:00Z

It seems to pull onto master fine...

Yes, because the last commit here is a merge with master where I throw away (that's the ours above) any conflicting changes from master.

reedy

Seems to work fine! Few related changes we can make to reduce the amount of things being returned by the SQL queries etc

Comments about foramtting mostly left for my reference... I'm happy to rebase to include the lost changes in the merge commit when ther other minor issues have been fixed up.

openflights.js

reedy · 2023-07-15T13:33:12Z

php/stats.php

@@ -22,12 +24,12 @@
    $sth->execute([$trid]);
    $row = $sth->fetch();
    if (!$row) {
-        die('Error;' . _("Trip not found."));
+        die(json_encode(["error" => ("Trip not found.")]));


Localistion dropped? _( has become (; brackets aren't adding anything in the current form

Could potentially use json_error() from helper.php.

I definitely need to do something here but I'm not trying to remove the translation. There is a gettext call for this string (for whatever is sent in the error prop in the UI now (line 2277). It works right now because we already have the string in the catalogue but the missing _ is a problem. However, I don't want to do localization here in this file.

I definitely don't want to use json_error (and json_success) because, IMO, data endpoints should not send back localized messages. That's a presentation layer concern.

I'm thinking of defining _ as the identity function in here so strings can be properly marked for localization in the UI.

It would be better having message codes, rather than strings passed wholesale to the localisation functions...

But I think that gets a little harder, changes how the defaults are defined (and found in source code) and a whole other host of problems.

So changes the workflow in one way or another too.

I'm thinking of defining _ as the identity function in here so strings can be properly marked for localization in the UI.

Which is what locale.php does for if !$OF_USE_LOCALES... And as long as no other PHP file includes stats.php, shouldn't cause an issue.

At some point, these want to become proper classes (especially if we go down the route of using a proper router etc), but that's a concern for down the road.

These could return an error code (and an english string? Ideally...), and then have the mapping from code => message in the JS and gettext should pick them up from there instead. And do the localisation there too.

And/or forcing a locale of en_us in standalone scripts like this, so the localisation function just returns the string anyway.

Not that json_success/json_error do any sort of code, just a status number...

php/stats.php

reedy · 2023-07-15T17:16:08Z

It seems to pull onto master fine...

Yes, because the last commit here is a merge with master where I throw away (that's the ours above) any conflicting changes from master.

I was referring to my previous comment

GitHub doesn't think they are resolved and there are currently conflicts...

And is (still) showing

This branch cannot be rebased due to conflicts

It might just be whatever git implementation github is using, sucking. I know jgit won't do trivial stuff that the "normal" git does fine/trivially with...

chrisrosset · 2023-07-15T17:19:48Z

It seems to pull onto master fine...

Yes, because the last commit here is a merge with master where I throw away (that's the ours above) any conflicting changes from master.

I was referring to my previous comment

GitHub doesn't think they are resolved and there are currently conflicts...

And is (still) showing

This branch cannot be rebased due to conflicts

It might just be whatever git implementation github is using, sucking. I know jgit won't do trivial stuff that the "normal" git does fine/trivially with...

Yeah, it can't be rebased which is why I merged master back here (throwing some changes away as a conflict resolution strategy). If you switch rebase to squash in the UI and you'll see it's happy to merge.

reedy

So really only just the localisation string issue from the PHP files to resolve at this point.

Then squash the changes to something you're happy with.

I can then rebase and restore the formatting changes, and should be good to put onto master :)

Which should also make it easier for me to fix a last couple of cases (on a patch ontop) that were left behind too...

openflights.js

reedy · 2023-07-15T17:19:30Z

openflights.js

+  table +=
+    "<tr>" +
+    `<td>${gt.gettext("Around the world")}</td>` +
+    `<td>${(distance / EARTH_CIRCUMFERENCE).toFixed(2)}x</td>` +


Another later TODO is possibly not having these defined different in both PHP and JS.... Possibly have PHP pass the constant value through when needed for calculations...

reedy · 2023-07-15T17:34:08Z

php/stats.php

@@ -22,12 +24,12 @@
    $sth->execute([$trid]);
    $row = $sth->fetch();
    if (!$row) {
-        die('Error;' . _("Trip not found."));
+        die(json_encode(["error" => ("Trip not found.")]));


It would be better having message codes, rather than strings passed wholesale to the localisation functions...

But I think that gets a little harder, changes how the defaults are defined (and found in source code) and a whole other host of problems.

So changes the workflow in one way or another too.

I'm thinking of defining _ as the identity function in here so strings can be properly marked for localization in the UI.

Which is what locale.php does for if !$OF_USE_LOCALES... And as long as no other PHP file includes stats.php, shouldn't cause an issue.

At some point, these want to become proper classes (especially if we go down the route of using a proper router etc), but that's a concern for down the road.

These could return an error code (and an english string? Ideally...), and then have the mapping from code => message in the JS and gettext should pick them up from there instead. And do the localisation there too.

And/or forcing a locale of en_us in standalone scripts like this, so the localisation function just returns the string anyway.

Not that json_success/json_error do any sort of code, just a status number...

chrisrosset · 2023-07-15T18:30:17Z

Made the _ change and squashed on this branch as requested.

reedy · 2023-07-15T20:02:10Z

php/stats.php

 include_once 'db_pdo.php';
 include_once 'helper.php';
 include_once 'filter.php';

+// Mark strings as localizable but don't localize.
+function _($str) { return $str; }


This actually causes breakages on my dev install (and therefore presumably would also in production).

"PHP message: PHP Fatal error: Cannot redeclare _() in /var/www/openflights/php/stats.php on line 11

But I haven't explicitly installed the PHP gettext extension, I believe. Just gettext for the CLI commands...

# dpkg -S /usr/lib/php/20220829/gettext.so php8.2-common: /usr/lib/php/20220829/gettext.so # dpkg -S /usr/lib/php/20190902/gettext.so php7.4-common: /usr/lib/php/20190902/gettext.so

And that's from the sury packages..

# dpkg -l | grep php | grep common ii php-common 2:93+ubuntu22.04.1+deb.sury.org+2 all Common files for PHP packages ii php7.4-common 1:7.4.33-6+ubuntu22.04.1+deb.sury.org+1 amd64 documentation, examples and common module for PHP ii php8.2-common 8.2.8-1+ubuntu22.04.1+deb.sury.org+1 amd64 documentation, examples and common module for PHP

I did set $OF_USE_LOCALES = true; to be able to test things.

If I set $OF_USE_LOCALES = false;, I get

PHP message: PHP Fatal error: Cannot redeclare _() in /var/www/openflights/php/locale.php on line 28

Looking at my bash history, I don't seem to have explicitly installed it either.

But looking again at the INSTALL and #1206 neither explicitly install any of the php-common packages...

https://packages.ubuntu.com/jammy/amd64/php8.1 depends on https://packages.ubuntu.com/jammy/amd64/php8.1-common which depends on https://packages.ubuntu.com/jammy/amd64/php-common

# apt-cache --installed rdepends php7.4-common php7.4-common Reverse Depends: php7.4-zip php7.4-xml php7.4-readline php7.4-opcache php7.4-mysql php7.4-mbstring php7.4-json php7.4-gd php7.4-fpm php7.4-curl php7.4-cli php7.4 php-common # apt-cache --installed rdepends php8.2-common php8.2-common Reverse Depends: php8.2-zip php8.2-xml php8.2-xdebug php8.2-readline php8.2-opcache php8.2-mysql php8.2-mbstring php8.2-gd php8.2-fpm php8.2-curl php8.2-cli # apt-cache --installed rdepends php-common php-common Reverse Depends: php8.2-zip php8.2-xml php8.2-xdebug php8.2-readline php8.2-opcache php8.2-mysql php8.2-mbstring php8.2-gd php8.2-curl php8.2-common php7.4-zip php7.4-xml php7.4-readline php7.4-opcache php7.4-mysql php7.4-mbstring php7.4-json php7.4-gd php7.4-curl php7.4-common php-zip php-xdebug php-mbstring php-json php-zip php-xdebug php-mbstring php-json

So you should have installed it on your Docker install too...

I'm guessing either something has changed with packaging (since whatever version the site was previously running on), as it's there in the default package on 22.04 (might need to refresh a few times, packages.ubuntu.com is giving intermittant HTTP 500)...

Does it work for you?

What do you have $OF_USE_LOCALES set to?

As far as I can tell... any calls to php/stats.php will be broken whichever way you've got it set...

And almost any other request (just loading the site) will be broken if you have $OF_USE_LOCALES = false; too...

Oof, yeah, it doesn't work. I admit I assumed it was fine based on locale.php already doing it. Ugh. And turning off locales breaks my local version as well.

I dunno if $OF_USE_LOCALES should be deprecated/removed, based on the existence of the apparent existence in default PHP...

Not stuff for this commit, but:

openflights/php/config.php.sample

Line 21 in 5e0fae7

// True requires PHP gettext extension, false works only if the extension is *not* installed

wants updating...

composer runs would actually fail if the "extension" wasn't loaded (unless you tried to use --ignore-platform-reqs) -

openflights/composer.lock

Line 4346 in 5e0fae7

"ext-gettext": "*",

https://github.com/jpatokal/openflights/blob/0f1adfc/php/locale.php#L28-L30 should either be removed (because of the composer requirement), or guarded with a !function_exists('_') call for it's definition (I doubt it's possible for ext-gettext to be apparently existing (ignoring --ignore-platform-reqs), but the function to not)...

Some distro's do do some weird stuff regarding packaging of php extensions... Some are included, some are pulled out etc...

But either way, both shouldn't be able to be true.

$OF_USE_LOCALES maybe/probably still has some value, basically unconditionally forcing en_US for all requests...

Heh, the config.php.sample comment is from c7b2cdf (May 2013)...

Can you just set $locale = "en_US.utf8"; at the top of the file?

Which, in theory, should "force" "en_US" for all _() calls in the file...

Can you just set $locale = "en_US.utf8"; at the top of the file?

Which, in theory, should "force" "en_US" for all _() calls in the file...

How would that work? Wouldn't locale.php override it with whatever is in $_SESSION?

chrisrosset · 2023-07-15T22:22:34Z

The += change also seems to have broken the API response.

reedy · 2023-07-15T22:32:10Z

The += change also seems to have broken the API response.

What's it complaining about?

It should work fine (we shouldn't have duplicate keys to worry about...), and shouldn't need to be using array_merge()...

reedy · 2023-07-15T22:34:21Z

php > $response = [ "total" => [ "segments" => 0 ] ];
php > $response["total"] += [ "somethingelse" => "text" ];
php > var_dump( $response );
array(1) {
  ["total"]=>
  array(2) {
    ["segments"]=>
    int(0)
    ["somethingelse"]=>
    string(4) "text"
  }
}

php/stats.php

reedy · 2023-07-15T22:35:22Z

Won't like you trying to use an array as a key :)

chrisrosset · 2023-07-15T22:58:36Z

I figured the [] problem first but then had to discover += doesn't override values for existing keys... I set total segments at the very top because it allows the UI to easily determine we don't have any flights. Fixed it all now.

Still testing locale stuff now.

chrisrosset · 2023-07-15T23:58:36Z

I'm not sure what's going on but my PHP installation is not actually translating any strings right now. JS is fine...

reedy · 2023-07-16T00:13:42Z

Anywhere? Or just this file?

You did re-add the include/require and drop the session start?

chrisrosset · 2023-07-16T00:18:28Z

Anywhere? Or just this file?

You did re-add the include/require and drop the session start?

Anywhere rendered server-side.

I restored the locale.php include and removed session_start. I did run locale-gen before and all (and JS works). I think I must have assumed the language I chose had an incomplete translation.

chrisrosset · 2023-07-16T00:21:16Z

php/stats.php

@@ -1,6 +1,7 @@
 <?php

 include_once 'locale.php';
+include_once 'config.php';


We use $OF_DEMO_UID so we shouldn't rely on a transitive include from locale.php.

chrisrosset · 2023-07-16T00:22:42Z

php/stats.php

@@ -22,12 +23,12 @@
    $sth->execute([$trid]);


I can't comment on line 17 because I haven't changed it (I hate this) but I think the existing $uid == 1 check is wrong... I think it's written with an assumption that you can only reach this from the demo screen.

chrisrosset · 2023-07-16T00:26:31Z

OK, gettext the PHP extension needs enabling in fpm/php.ini apparently.

chrisrosset · 2023-07-16T01:43:33Z

Alright, I'm not sure how to disable translation of the error messages but I'm also not super bothered about it happening... for now. It's also consistent with what I've done in top10.php. We should revisit our error-handling philosophy in general in the near future as we upgrade endpoints.

reedy · 2023-07-16T08:09:51Z

https://stackoverflow.com/questions/24200471/php-gettext-without-relying-setlocale

chrisrosset · 2023-07-16T15:48:12Z

https://stackoverflow.com/questions/24200471/php-gettext-without-relying-setlocale

Not sure which part I'm supposed to look at? In any case, I think we're OK here for now.

reedy · 2023-07-16T16:00:46Z

Just call setlocale(LC_MESSAGES, "en_US");

Shouldn't affect any other scope... or change session stuff

Reworks the changes from 6369e67 Fixes jpatokal#1344

chrisrosset changed the title ~~Stats json~~ Return JSON-encoded results from stats.php Jul 15, 2023

chrisrosset marked this pull request as ready for review July 15, 2023 03:43

reedy requested changes Jul 15, 2023

View reviewed changes

reedy reviewed Jul 15, 2023

View reviewed changes

chrisrosset force-pushed the stats-json branch from 5ed79ec to 719b0ea Compare July 15, 2023 18:27

reedy requested changes Jul 15, 2023

View reviewed changes

php/stats.php Outdated Show resolved Hide resolved

php/stats.php Outdated Show resolved Hide resolved

chrisrosset force-pushed the stats-json branch from 719b0ea to 45906e7 Compare July 15, 2023 22:36

chrisrosset force-pushed the stats-json branch from 45906e7 to b46a2b5 Compare July 16, 2023 00:20

chrisrosset commented Jul 16, 2023

View reviewed changes

chrisrosset requested a review from reedy July 16, 2023 01:43

chrisrosset and others added 2 commits July 17, 2023 19:55

Return JSON-encoded results from stats.php

1876df8

Restore number formatting on Analyze page

2c83c5a

Reworks the changes from 6369e67 Fixes jpatokal#1344

reedy force-pushed the stats-json branch from b46a2b5 to 2c83c5a Compare July 17, 2023 20:10

reedy merged commit 108833e into jpatokal:master Jul 17, 2023

reedy mentioned this pull request Jul 17, 2023

"Class by distance" chart does not display the units on the popup/tooltip #1363

Closed

Return JSON-encoded results from stats.php #1345

Return JSON-encoded results from stats.php #1345

Conversation

chrisrosset commented Jul 15, 2023

chrisrosset commented Jul 15, 2023

reedy commented Jul 15, 2023 • edited Loading

reedy commented Jul 15, 2023

chrisrosset commented Jul 15, 2023

reedy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reedy commented Jul 15, 2023

chrisrosset commented Jul 15, 2023

reedy left a comment

Choose a reason for hiding this comment

reedy Jul 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisrosset commented Jul 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reedy Jul 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reedy Jul 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisrosset commented Jul 15, 2023

reedy commented Jul 15, 2023

reedy commented Jul 15, 2023

reedy commented Jul 15, 2023

chrisrosset commented Jul 15, 2023

chrisrosset commented Jul 15, 2023

reedy commented Jul 16, 2023

chrisrosset commented Jul 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisrosset commented Jul 16, 2023

chrisrosset commented Jul 16, 2023

reedy commented Jul 16, 2023

chrisrosset commented Jul 16, 2023

reedy commented Jul 16, 2023 • edited Loading

Return JSON-encoded results from `stats.php` #1345

Return JSON-encoded results from `stats.php` #1345

reedy commented Jul 15, 2023 •

edited

Loading

reedy Jul 15, 2023 •

edited

Loading

reedy Jul 15, 2023 •

edited

Loading

reedy Jul 15, 2023 •

edited

Loading

reedy commented Jul 16, 2023 •

edited

Loading