onotes doctrine impl #453

MatthewVita · 2017-01-26T04:18:30Z

No description provided.

MatthewVita · 2017-01-27T05:18:58Z

Important bit on Doctrine that tripped me up:

It is not possible to use join columns pointing to non-primary keys. Doctrine will think these are the primary keys and create lazy-loading proxies with the data, which can lead to unexpected results. Doctrine can for performance reasons not validate the correctness of this settings at runtime but only through the Validate Schema command.

cc: @juggernautsei, @bradymiller

MatthewVita · 2017-01-27T05:23:57Z

Okay, so here's the @join in action (really silly example, of course):

$database = \common\database\Connector::Instance();
$entityManager = $database->entityManager;
$repository = $entityManager->getRepository('\entities\ONote');
$oNotes = $repository->findAll();

foreach($oNotes as $note) {
  echo '<h2>User: ' . $note->getUser()->getFname() . ' (' . $note->getUser()->getTaxonomy() . ')</h2>';
  echo '<h3 style="color:green">' . $note->getBody() . '</h3><br />';
}

Notice how getUser() is returning the information from the other table via the annotations. ****

Btw if the basic repository methods that EntityRepository gives us aren't enough for more complex cases, there is http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/dql-doctrine-query-language.html or this abstraction on top of it: http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/query-builder.html

onotes has:

    /**
     * @ManyToOne(targetEntity="User", inversedBy="onote"))
     * @JoinColumn(name="user", referencedColumnName="username")
     */
    private $user;

user has:

    /**
     * @OneToMany(targetEntity="ONote", mappedBy="user")
     */
    private $onotes;

cc: @juggernautsei, @bradymiller

juggernautsei · 2017-01-27T12:15:00Z

Not so silly of an example. It clarifies what I was wondering. Because in some of the past code the sql statements were still being used but in the example given no sql is being used. Great! So the doctrine is negotiating the connection and request for data with the developer having to type in sql commands to get data. As it should be.

Now what do I do with the entities, reposistories, and services I've created? Not seeing the next steps to take yet since the direction of development has changed from what I am use to. Help me switch gears and change the order of development.

MatthewVita · 2017-01-28T17:20:44Z

@juggernautsei,

Not so silly of an example. It clarifies what I was wondering.

I'm glad that it was helpful.

without the developer having to type in sql commands to get data.

In most cases the methods of the extended Doctrine\ORM\EntityRepository class will be good enough. However, sometimes more complex queries have to be written in a Doctrine-friendly way called "DQL" http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/dql-doctrine-query-language.html. I don't have an example of where this would be useful in our code yet, but I'm certain we'll run into such a case. Here's an example of how this looks ( @bradymiller will most likely be interested in this as well):

namespace repositories;

use entities\Version;
use Doctrine\ORM\EntityRepository;

class ONoteRepository extends EntityRepository {
    public function findAll() {
       $sql  = "SELECT o ";
       $sql .= "FROM entities\\ONote o ";
       $sql .= "JOIN entities\\User u ";
       $sql .= "WITH o.user = u.username";

       return $this->_em->createQuery($sql)->getResult();
     }
}

This is very cool because it still uses the entities and hydrates the JOIN'ed User entity with appropriate data. Obviously, this is a useless example because the extended EntityRepository class provides a findAll method that will do just this.

Btw, this concept is from Hibernate with something called "HQL".

Now what do I do with the entities, reposistories, and services I've created? Not seeing the next steps

Great question. Let's start out with just the autoloading and SELECT pieces. Open up openemr/interface/product_registration/product_registration_service.php and add the following field and update the constructor:

private $repository;

public function __construct() {
  $database = \common\database\Connector::Instance();
  $entityManager = $database->entityManager;
  $this->repository = $entityManager->getRepository('\entities\ProductRegistration');
}

Now you can edit the following:

public function getProductStatus() {
  //$row = sqlQuery('SELECT * FROM product_registration LIMIT 1');
  $productRegistration = $repository->findFirst();
  ...
  ...

findFirst is actually not provided by EntityRepository, so add the following method to your repository:

public function findFirst() {
  $results = $this->_em->getRepository($this->_entityName)->findAll();
    if (!empty($results)) {
      return $results[0];
    }

    return null;
}

...once this is working, let's do the INSERT piece next.

Thanks,
Matthew

MatthewVita · 2017-01-29T02:50:57Z

@bradymiller Please review :). I tested in frames and tabs mode.

cc: @juggernautsei

bradymiller · 2017-01-29T11:10:05Z

interface/main/onotes/office_comments.php

 ?>

 <html>
 <head>

 <link rel="stylesheet" href="<?php echo $css_header;?>" type="text/css">
+<link rel="stylesheet" href="<?php echo $GLOBALS['assets_static_relative'] ?>/bootstrap-3-3-4/dist/css/bootstrap.min.css">


When bring in bootstrap style, also need to support the RTL folks by bringing in the bootstrap rtl after it:

<link rel="stylesheet" href="<?php echo $GLOBALS['assets_static_relative'] ?>/bootstrap-3-3-4/dist/css/bootstrap.min.css"> <?php if ($_SESSION['language_direction'] == 'rtl') { ?> <link rel="stylesheet" href="<?php echo $GLOBALS['assets_static_relative'] ?>/bootstrap-rtl-3-3-4/dist/css/bootstrap-rtl.min.css"> <?php } ?>

Good to know.

Does this include the CSS file as well? Is it just the js?

just the css file:
https://github.com/morteza/bootstrap-rtl/blob/master/README.md#bootstrap-3-rtl-theme

Great. Updated.

bradymiller · 2017-01-29T11:27:28Z

library/onotes.inc

@@ -1,76 +0,0 @@
-<?php
-/**


fare thee well onotes.inc!
:)

that is because the doctrine?

yep, Matthew brought all the code into the pertinent services class, which is where it belongs. It is another small step in OpenEMR's migration from legacy code to modernized classes.

bradymiller · 2017-01-29T11:30:07Z

repositories/onote_repository.php

+     * @return The single note.
+     */
+    public function findNoteById($id) {
+        $result = $this->_em->getRepository($this->_entityName)->findOneBy(array("id" => $id));


This may be a stupid question. Just checking that the $id gets escaped for sql (and for that matter, all the parameters in queries); I am assuming it does, but just checking.

Yup, everything is parameterized:

INSERT example:

2017-01-31 07:14:35 [DEBUG] \services\ONoteService - Adding new office note 2017-01-31 07:14:35 [TRACE] \common\database\Auditor - sql: "START TRANSACTION" 2017-01-31 07:14:35 [TRACE] \common\database\Auditor - sql: INSERT INTO onotes (date, body, groupname, activity, user) VALUES (?, ?, ?, ?, ?) 2017-01-31 07:14:35 [TRACE] \common\database\Auditor - sql: "COMMIT" 2017-01-31 07:14:35 [DEBUG] \services\ONoteService - Added new office note 26

SELECT WHERE example:

2017-01-31 07:16:56 [DEBUG] \services\ONoteService - Getting 0 onotes with filters: 0 10 2017-01-31 07:16:56 [TRACE] \common\database\Auditor - sql: SELECT t0.id AS id_1, t0.date AS date_2, t0.body AS body_3, t0.groupname AS groupname_4, t0.activity AS activity_5, t0.user AS user_6 FROM onotes t0 WHERE t0.activity = ? ORDER BY t0.date DESC LIMIT 10 OFFSET 0

bradymiller · 2017-01-29T11:32:30Z

repositories/onote_repository.php

+            $criteria,
+            array("date" => "DESC"),
+            $limit,
+            $offset


2 related questions to above. Do the limit and offset get escaped for downstream sql query?

Here's an example of the LIMIT and OFFSET:

2017-01-31 07:19:18 [DEBUG] \services\ONoteService - Getting 1 onotes with filters: 20 10 2017-01-31 07:19:18 [TRACE] \common\database\Auditor - sql: SELECT t0.id AS id_1, t0.date AS date_2, t0.body AS body_3, t0.groupname AS groupname_4, t0.activity AS activity_5, t0.user AS user_6 FROM onotes t0 WHERE t0.activity = ? ORDER BY t0.date DESC LIMIT 10 OFFSET 20

...they are not escaped. I'm not sure how to do that in SQL. I believe if a non-numeric value gets in there, the query will be invalid and not work.

UPDATE: just hardcoded 'foobar' in for the $limit and the query didn't run.

Just need to ensure sql-injection can't happen in those cases. We needed to create specialized functions for some of the more unusual cases where user/client input can get into the query:
https://github.com/openemr/openemr/blob/master/library/formdata.inc.php#L48 (also used this for offset)
https://github.com/openemr/openemr/blob/master/library/formdata.inc.php#L64
https://github.com/openemr/openemr/blob/master/library/formdata.inc.php#L82
https://github.com/openemr/openemr/blob/master/library/formdata.inc.php#L138

Thanks for all of the examples. We will have to keep these in mind with future Doctrine PRs.

Turns out that columns, tables, where params, etc are all safe when using straight EntityRepository methods. However, I dove through the code and found that the LIMIT and OFFSET don't appear to be secured. Because this is something I'm passing in as a variable, I simply am validating that the $limit and $offset are numeric with is_numeric(...).

Should be good.

bradymiller · 2017-01-29T11:38:28Z

vendor/autoload.php

@@ -2,6 +2,6 @@

 // autoload.php @generated by Composer

-require_once __DIR__ . '/composer' . '/autoload_real.php';
+require_once __DIR__ . '/composer/autoload_real.php';


Just checking that you are only doing the following command in composer:
composer dump-autoload -o
(just noting that when you process via composer always consistently bring in some changes like above; not a big deal and will basically just likely keep flip flopping this stuff depending on who runs the composer update command; just want to make sure you aren't using any custom composer settings or anything like that)

interesting, it looks like changes may stem from apcu settings in php:
composer/composer@6d4e60b
(I think it's a nonissue though, since this works fine on my system)

I "cheat" and do reverse searches for "openemr_composer" in my terminal history to run this all the time:

composer install && ./vendor/bin/phing vendor-clean && rm -rf vendor/phing && rm -rf vendor/bin/phing && composer dump-autoload -o # openemr_composer

This is what I used to check in this PR. Is something off with the command?

command looks good and the code works on my os, so guessing not an issue.

bradymiller · 2017-01-29T11:46:04Z

vendor/composer/autoload_real.php

@@ -23,7 +23,7 @@ public static function getLoader()
        self::$loader = $loader = new \Composer\Autoload\ClassLoader();
        spl_autoload_unregister(array('ComposerAutoloaderInit22ddb69348c7ed922c96325249cef3d0', 'loadClassLoader'));

-        $useStaticLoader = PHP_VERSION_ID >= 50600 && !defined('HHVM_VERSION');
+        $useStaticLoader = PHP_VERSION_ID >= 50600 && !defined('HHVM_VERSION') && (!function_exists('zend_loader_file_encoded') || !zend_loader_file_encoded());


this seems odd to search for a zend_loader_file. is this a custom composer setting?

I "cheat" and do reverse searches for "openemr_composer" in my terminal history to run this all the time:

composer install && ./vendor/bin/phing vendor-clean && rm -rf vendor/phing && rm -rf vendor/bin/phing && composer dump-autoload -o # openemr_composer

This is what I used to check in this PR. Is something off with the command?

command looks good and the code works on my os, so guessing not an issue.

bradymiller · 2017-01-29T11:50:16Z

Very cool code and gui improvements. Just completed the review(see my comments above).

juggernautsei

I see that you are replacing the < table > tags with a div. Right?
And you added the bootstrap to the page ref the new location for the page layout design.

MatthewVita · 2017-01-31T12:27:00Z

I see that you are replacing the < table > tags with a div. Right?

@juggernautsei Yeah, I believe it simplifies things because the notes in this view don't need to be a table because they are essentially "cards".

Very cool code and gui improvements. Just completed the review(see my comments above).

@bradymiller I really appreciate it. I just left my remarks.

MatthewVita · 2017-02-01T03:11:15Z

@bradymiller I addressed your recent comments and I think we're ready/close to merging!

Thanks!

bradymiller · 2017-02-01T07:37:50Z

Code looks good, so I just brought this into the codebase. Thanks for the contribution! -brady

MatthewVita added the Backend Modernization Project label Jan 26, 2017

MatthewVita changed the title ~~WIP: onotes doctrine impl~~ onotes doctrine impl Jan 29, 2017

MatthewVita added the Status: Needs Review label Jan 29, 2017

MatthewVita requested a review from bradymiller January 29, 2017 02:50

bradymiller reviewed Jan 29, 2017

View reviewed changes

bradymiller self-assigned this Jan 29, 2017

bradymiller added Status: Needs Work and removed Status: Needs Review labels Jan 29, 2017

juggernautsei reviewed Jan 30, 2017

View reviewed changes

MatthewVita added 4 commits January 31, 2017 21:53

onotes doctrine impl

026361a

updated composer

84ec6a6

onotes ui improvement

854e25d

updated composer with master

61f583b

MatthewVita added Status: Needs Review and removed Status: Needs Work labels Feb 1, 2017

bradymiller merged commit 0d31c2e into openemr:master Feb 1, 2017

MatthewVita removed the Status: Needs Review label Feb 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

onotes doctrine impl #453

onotes doctrine impl #453

MatthewVita commented Jan 26, 2017

MatthewVita commented Jan 27, 2017 •

edited

MatthewVita commented Jan 27, 2017 •

edited

juggernautsei commented Jan 27, 2017

MatthewVita commented Jan 28, 2017

MatthewVita commented Jan 29, 2017

bradymiller Jan 29, 2017 •

edited

juggernautsei Jan 30, 2017

MatthewVita Jan 31, 2017

bradymiller Jan 31, 2017

MatthewVita Feb 1, 2017

bradymiller Jan 29, 2017

juggernautsei Jan 30, 2017

bradymiller Jan 30, 2017

MatthewVita Jan 31, 2017

bradymiller Jan 29, 2017

MatthewVita Jan 31, 2017

bradymiller Jan 29, 2017

MatthewVita Jan 31, 2017 •

edited

bradymiller Jan 31, 2017

MatthewVita Feb 1, 2017

bradymiller Jan 29, 2017

bradymiller Jan 29, 2017

MatthewVita Jan 31, 2017

bradymiller Jan 31, 2017

MatthewVita Feb 1, 2017

bradymiller Jan 29, 2017

MatthewVita Jan 31, 2017

bradymiller Jan 31, 2017

MatthewVita Feb 1, 2017

bradymiller commented Jan 29, 2017

juggernautsei left a comment •

edited

MatthewVita commented Jan 31, 2017

MatthewVita commented Feb 1, 2017

bradymiller commented Feb 1, 2017

onotes doctrine impl #453

onotes doctrine impl #453

Conversation

MatthewVita commented Jan 26, 2017

MatthewVita commented Jan 27, 2017 • edited

MatthewVita commented Jan 27, 2017 • edited

juggernautsei commented Jan 27, 2017

MatthewVita commented Jan 28, 2017

MatthewVita commented Jan 29, 2017

bradymiller Jan 29, 2017 • edited

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

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

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

MatthewVita Jan 31, 2017 • edited

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

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

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

bradymiller commented Jan 29, 2017

juggernautsei left a comment • edited

Choose a reason for hiding this comment

MatthewVita commented Jan 31, 2017

MatthewVita commented Feb 1, 2017

bradymiller commented Feb 1, 2017

MatthewVita commented Jan 27, 2017 •

edited

MatthewVita commented Jan 27, 2017 •

edited

bradymiller Jan 29, 2017 •

edited

MatthewVita Jan 31, 2017 •

edited

juggernautsei left a comment •

edited