picks for patch2 #7488
picks for patch2 #7488
Conversation
* Add new module loaded event - Notify any listeners that modules are loaded. - Inlude modules that failed and reason in enabled modules event return array. * Remove disable module on unreadable bootstrap after 3 tries. Document license and copyrights * Redesign Weno pharmacy search to add additional options Etherfax Ajax/Fetch refactors for readabilty and funtional * more weno search refactor * redevelop csv pharmacy import using league/csv for better coverage and validation rework search rework and refactor several classes for readability and consoladation remove one class from refactor remove old csv import * modify search filter warning fix * more neat search stuff * an update to implement pharmacy select from widget or Choices. new script to update assigned pharmacies from weno widget persist search filters to be reused. * remove warnings alert since not needed * verbiage change add refresh after pharmacy update * Fix acl to only allow widget if demo write vecause must add pharmacies. fix provider password for prescription log. safegaurd session on ajax * cleanup add upgrade sql script * php warning fixes (cherry picked from commit e873bb6)
| } | ||
| echo text(json_encode($return_arr)); | ||
| echo json_encode($return_arr); |
There was a problem hiding this comment.
Was text() call breaking something here? Hopefully not since we do this frequently (html escape a json_encoding value when echo'ing into html context)
There was a problem hiding this comment.
Yes there can be issues escaping with text here. Main issue is this data comes from database and has special characters like '&' that can be part of a pharmacy name and thus you wind up with mangled(noticeable) escaped characters in select.
I let it slide but got called out about it twice but Weno testers.
I've had other times where wrapping with text can be an issue but not enought to be concerned and just have to be aware where used.
| ); | ||
| } | ||
| } | ||
| echo text(json_encode($return_arr)); | ||
| echo json_encode($return_arr); |
There was a problem hiding this comment.
same question here on above regarding the text() call
| `State_Wide_Mail_Order` varchar(15) NOT NULL, | ||
| `Mail_Order_US_State_Serviced` varchar(255) DEFAULT NULL, | ||
| `Mail_Order_ US_Territories_Serviced` varchar(255) DEFAULT NULL, | ||
| `On_WENO` tinytext DEFAULT NULL, |
There was a problem hiding this comment.
best to have no modifiers for 'text' fields
| } | ||
|
|
||
| // Database configuration. Create connection. | ||
| $connect = new mysqli($GLOBALS["host"], $GLOBALS["login"], $GLOBALS["pass"], $GLOBALS["dbase"]); |
There was a problem hiding this comment.
Just checking why you needed to do use direct mysqli calls.
Couple issues that will come up (can be dealt with with example in how done in setup.php script):
- Setting of encoding
- Breaking if mysql is set up to enforce encryption over wire
There was a problem hiding this comment.
Actually, code is in Installer.class.php and not setup.php
There was a problem hiding this comment.
yeah I simply forgot to go back to setting names. I was going to used same as we used in portal and got side tracked but you're correct this can cause issues that could be very hard to track down.
The reasons for direct management are of concern for me and something we need to address. 1. Our wrappers are slow. 2. If I was just importing from UI I wouldn't care too much however I've found out how easy running a database intensive script in background services can cause problem for front facing foreground runtime. It can hang foreground enough to be noticed by user where they can start clicking randomly while trying to figure out why app is hung.
The performance difference is quit noticeable by a factor of 50% which I benched marked.
I'm going to once again warn everyone that we need to take a close look at what we run in these background tasks.
My experience looking at a couple production sites that have several modules turn on plus our house keeping tasks like the UUID update, direct email polling, X12 severely affect openemr runtime from UI POV.
What I've done here for this query intensive transaction needs to be an example of doing the same for some of our other queries that affect performance.
Having our current wrappers are/were a nice convenience for the type of workloads expected 10 years ago but as openemr grows with far greater data processing requirements and moving more to the enterprise, we have to/must put an effort into profiling and tuning openemr if we are going to compete for large enterprise requirements.
I haven't even brough into the affect API's will have as well.
I just don't want to be the only admin that's investigating this stuff or not worried about. We have to get used to having much larger data sets for testing with more realistic options turned on.
Anyway thanks for the catch Brady. I knew about but simply forgot!
There was a problem hiding this comment.
@bradymiller Can you help me out with this? Not sure best way to open connection and my brain is so tired...
There was a problem hiding this comment.
never mind @bradymiller I found a way that's an easy compromise. Going to use the existing connection from globals
| `State_Wide_Mail_Order` varchar(15) NOT NULL, | ||
| `Mail_Order_US_State_Serviced` varchar(255) DEFAULT NULL, | ||
| `Mail_Order_ US_Territories_Serviced` varchar(255) DEFAULT NULL, | ||
| `On_WENO` tinytext DEFAULT NULL, |
There was a problem hiding this comment.
best for text fields to have no modifiers
There was a problem hiding this comment.
is supposed to be tinyint:)
Fixes #
Short description of what this resolves:
Changes proposed in this pull request: