CORE-POS / IS4C

Cooperative Operational Retail Environment
http://www.core-pos.com
GNU General Public License v2.0
63 stars 44 forks source link

Newly installed lane won't transmit data to server #1194

Open scannerdarkly opened 1 year ago

scannerdarkly commented 1 year ago

So our newly installed CORE-POS lane doesn't transmit transaction data to the server. The old lane does, just fine, so it's not a server problem.

Definitely checked all the obvious stuff:

  1. Network connectivity is solid (e.g. lane and server can each ping and ssh to the other)
  2. parameters table in the new lane has same mServer, mUser, and mPass as the old (successfully transmitting) lane
  3. New lane's on-screen connection icon is the green “connected” dot
  4. New lane transactions complete normally with no error messages
  5. New lane receipt reprints work 100%
  6. New lane end-of-day report shows all its own transactions

But the data never uploads to the server at all. dlog_15 has nothing from the new lane's transactions, and the server-based reports show nothing either.

The only interesting other symptom I'm observing is that when the new lane finishes a transaction, there's a slightly longer delay before the change is shown and the drawer opens. So it feels like some kind of server timeout.

What PHP module transmits CORE-POS's completed transaction data to the server upon the end of the transaction?

gohanman commented 1 year ago

A normal lane will never put anything into dlog_15, so I'm not sure what the significance of that would be.

What, if anything, gets logged? Both a timeout or a failed query should result in log entries.

scannerdarkly commented 1 year ago

lane.log, php-errors.log, queries.log are all empty.

My debug_lane.log is constantly filling up VERY quickly with compatibility notices of this basic form:

corepos[1315]: (debug) Return type of COREPOS\pos\parser\ParseResult::_function_name() should either be compatible with Iterator|Countable::function_name(): return_type_, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice Line 112, /CORE-POS/pos/is4c-nf/parser/ParseResult.php

BUT...

At the exact moment I finish a transaction, it briefly adds the following message exactly five times:

corepos[1315]: (debug) Function strftime() is deprecated Line 144, /CORE-POS/pos/is4c-nf/lib/TransRecord.php

and then this (just once):

corepos[1315]: (debug) Function strftime() is deprecated Line 220, /CORE-POS/pos/is4c-nf/lib/gui/BasicCorePage.php

before returning to the rapid-fire stream of compatibility notices.

Is TransRecord.php responsible for copying the transaction record to the server? What server table should I be watching for evidence of a successful update?

gohanman commented 1 year ago

The strftime deprecated messages indicate you're using PHP 8.1.0 or higher. That shouldn't be a problem per se, since they're just deprecations, but it's hard to tell what version you're actually using. The line number for TransReccord.php doesn't match the version-1.9 codebase and for that matter lib/gui didn't exist yet at that point, but it also doesn't align with the latest. I assume the first compatibility notice also has to be related to the PHP version but it's hard to say what without knowing what line 112 is in the codebase you're working with is.

Database.php(https://github.com/CORE-POS/IS4C/blob/master/pos/is4c-nf/lib/Database.php#L439-L496) is responsible for uploading data and it should be landing in dtransactions on the server side.

scannerdarkly commented 1 year ago

What version of PHP would be best for us to use? This change, at least, would be near-trivial.

gohanman commented 1 year ago

7.4 is the highest that's seen extensive use, at to my knowledge. But I'm pretty certain these notices have nothing to do with the uploading data issue.

scannerdarkly commented 1 year ago

I share your belief that they're unrelated — but it'll be nice to have fewer interfering signals as I read through the logs. I'll change this and check out a few other things and report back!

gohanman commented 1 year ago

Just for reference:

Fix for ParseResult notices: a458562

Eliminate deprecated strftime a3ff60f

scannerdarkly commented 1 year ago

I've found the problem: uploadtoServer() was never being called, and that's because testremote() was never being called, and that's because (in AjaxEnd.php) uploadAndReset() is seeing $this->session->get("testremote") return an empty string.

Under earlier versions of PHP, '' == 0 evaluates to true — but under PHP 8.x, '' == 0 evaluates to false! This means under PHP 8.x $this->session->get("testremote") must return either numeric or string 0 in order for uploadtoServer() to ever get called.

I replaced if ($this->session->get("testremote")==0) with if (!$this->session->get("testremote")) to return CORE-POS to behavior closer to pre-8.x. Should I check in this change to the codebase?

For extra resilience, I'm curious how I get $this->session->get("testremote") to return either numeric or string 0?

gohanman commented 1 year ago

For extra resilience, I'm curious how I get $this->session->get("testremote") to return either numeric or string 0?

The short answer is "you can't". The default behavior of the session handler on an unknown key is to return an empty string, and that can't be changed without rippling out other potential problems.

It looks like this is a historical artifact and the conditional could just be removed - nothing else checks or sets the testremote key (c8622db). But it's not 100% satisfactory as an explanation since Database::testremote isn't just called at the end of a transaction. It also gets called on login.

scannerdarkly commented 1 year ago

For extra resilience, I'm curious how I get $this->session->get("testremote") to return either numeric or string 0?

The short answer is "you can't". The default behavior of the session handler on an unknown key is to return an empty string, and that can't be changed without rippling out other potential problems.

Sorry for being ambiguous, I was asking if there's a way to truly set testremote to 0. Not asking to change the $this->session->get() behavior for unset keys — the latter change might have truly catastrophic effects.

But it's not 100% satisfactory as an explanation since Database::testremote isn't just called at the end of a transaction. It also gets called on login.

This smells like some refactoring would be ideal here, as testremote() does something much more substantial than just test the remote connection and so is (by my thinking) already confusingly named. But I don't know the architecture nearly well enough to suggest what refactoring would be most fitting.

gohanman commented 1 year ago

If you wanted to initialize testremote to 0, the normal place to do that would be in CoreState. But given nothing else would change it, I'm not sure why that would be better than just removing the conditional.

I agree the naming isn't ideal; these are some ancient corners of the codebase that have been named that way since OG IS4C. It wouldn't be a big deal to rename since it's only called in 3 places, but I'm not sure there's a concise better name. Beyond renaming I don't think there's a good refactor. On balance having the logic for deciding if the server is up and operational confined to one place is better than letting it spill out into all the call sites where logic could get out of sync.