Determine if using an ORM is appropriate #337

Open
MatthewVita opened this Issue Nov 3, 2016 · 78 comments

Projects

DONE in Modernization

4 participants

@MatthewVita
Member
MatthewVita commented Nov 3, 2016 edited

Looks like Doctrine is available in the codebase. It’s decent looking and compatible with Zend.

If ORM is a go, create issues for all major objects.

@MatthewVita MatthewVita self-assigned this Nov 3, 2016
@bradymiller
Member
bradymiller commented Nov 10, 2016 edited

Regarding sql, note @Wakie87 is in the midst of a very large project to migrate from adodb to PDO/aura :
https://github.com/Wakie87/openemr/commits/database_v4

ORM sounds like a very good starting point for the modernization project, which @sunsetsystems has been suggesting for several years(ie. creating classes for objects that interact with the database).

@bradymiller
Member

Regarding ORM, something that makes sense would be to just focus on 1 item to show an example of how it works. In the past, @sunsetsystems recommended that a nice simple example would be the version table/object:
https://sourceforge.net/p/openemr/discussion/oemr_501c3/thread/998b6170/#b9ab
(I hope I haven't confused DAO with ORM...)

@MatthewVita
Member
MatthewVita commented Nov 12, 2016 edited

Hi @bradymiller,

I am aware of the ongoing PDO work. I feel that that work is a nice intermediate step. Full ORM integration would be ideal. Moreover, it seems like Rod and others have expressed a need for that given the complexity of the database and, of course, I have an "agenda" to have the views/controllers/services/models separated with the modernization project.

I have been looking into Doctrine (http://www.doctrine-project.org/) which is an ORM that has a lot of Hibernate (http://hibernate.org/) influence. I am experienced with Hibernate and think it is a great pattern for ORMs.

Doing a simple example with the existing codebase would be a great start. I am just finishing up the research phase.

Will follow up on how that goes!

-m

@bradymiller
Member

Hi @MatthewVita ,
Agreed on all counts.
-brady

@juggernautsei
Contributor

Matthew,
with your "agenda" for the project to become a full MVC. Did you map out a plan to get there?
Are you planning on spending the man hours to rewrite the entire platform?
I'm just asking because the plan is concealed from view.

@MatthewVita
Member

@juggernautsei,

All we can do is work on one problem at a time. As far as the entire modernization project is concerned, we are only as strong as the number of people working on it. Obviously, we want to avoid "too many cooks in the kitchen", but at the moment there is a limited number of people with a limited amount of time. Hopefully, this will change when OEMR gets non-profit status and has grant funds to contract out development work. At that point, we should have a good backlog of achievable modernization tasks with well thought out development approaches.

For now, let's continue with the ORM work (recall that this task is "Determine if using an ORM is appropriate" - a more research-oriented task). I believe that Doctrine is a good choice and will work with both Zend and the existing code. Next step is to put a branch together that demonstrates the integration. Afterwards, we can task-out which data models will need integrated and identify folks to help out.

-m

@MatthewVita
Member

The version table will be a good starting point, as @bradymiller mentioned.

@MatthewVita
Member

@juggernautsei,

One more thing of note. With the two recent projects I ran for OpenEMR (Product Registration and Website Rework), I mapped out everything on the wiki:

You can see that I tasked the work out to be achievable as well as having an "owner" (aka the person working on it).

My point in mentioning this is that for both projects I started with a big list of the work to be done and basically found myself adding/removing/editing those tasks constantly. I don't think it was very productive. Therefore, building up a backlog of tasks as we go (whether through Github "projects" as we have here or through a task list similar to my examples above) is probably a good idea.

The Agile Manifesto is something that I am finding to be the best resource for most things nowadays: http://blog.itil.org/wp-content/uploads/2014/08/AgileManifesto.png

-m

@juggernautsei
Contributor

This helps a lot so that I can see the path also.
Since you are convinced of using Doctrine and I see no reason not to use Doctrine 2. I am going to devote some time to http://www.jasongrimes.org/2012/01/using-doctrine-2-in-zend-framework-2/.

This tutorial talks about how to merge the two platforms together. Since zend is in the code base.

Do you plans include using Zend framework for the MVC?

FYI, I just read your post before posting this.

@MatthewVita
Member
MatthewVita commented Nov 16, 2016 edited
  • openemr/version.php has the base $v_major, $v_minor, and $v_patch
  • openemr/admin.php queries the table
  • openemr/sql_upgrade.php updates the table
  • openemr/library/classes/Installer.class.php
  • openemr/sql_patch.php
@MatthewVita
Member

autoload_static.php is generated by composer and wires up Doctrine

@juggernautsei
Contributor

openemr/sql_upgrade.php queries the table

Not sure what you mean by this? Because sql_upgrade is just a href in the admin.php

@juggernautsei
Contributor
juggernautsei commented Nov 16, 2016 edited

Do you have this posted somewhere?
Did you just create the autoload_static.php? Where?

@MatthewVita
Member

openemr/sql_upgrade.php is doing the following:

  sqlStatement("UPDATE version SET v_major = '$v_major', v_minor = '$v_minor', " .
    "v_patch = '$v_patch', v_tag = '$v_tag', v_database = '$v_database'");
  sqlStatement("UPDATE version SET v_realpatch = '$v_realpatch'");
@MatthewVita
Member

and no I don't have a branch out there yet.

@juggernautsei
Contributor

Should I be doing the same of should I wait till you publish to follow after your work?

@bradymiller
Member

The autoload_static.php is in the current developments codebase and is part of composer which Scott brought into the project. This basically autoloads all the classes and is built by composer; it's very cool:
https://github.com/openemr/openemr/blob/master/vendor/composer/autoload_static.php
(check out the following wiki page to learn more about the ongoing composer project: http://www.open-emr.org/wiki/index.php/Composer )

version table is also used here:
library/classes/Installer.class.php
sql_patch.php

Just to note, the version.php does not touch the version sql table.

Very cool to see the thought process of this.

@bradymiller
Member

@juggernautsei
To finish up the composer thought, here is where the autoloaded classes are brought into OpenEMR in the globals.php script:
https://github.com/openemr/openemr/blob/master/interface/globals.php#L192-L193

@MatthewVita
Member

@juggernautsei now that we've identified the areas that we'll need to change, we just have to get the composer stuff in line. Let's read the materials @bradymiller just sent over as well as that article about the basics of Doctrine (http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/tutorials/getting-started.html).

Also, I don't think we came to a conclusion about the project management piece. Are you advocating that we "task out" everything in the Wiki and/or Github Project Tasks in clear terms or do you want to "discover the needed tasks as we go" as is the case now? I'm fine with either approach but I will say I'm not convinced that the former provides a lot of value. I'm interested in your thoughts.

Thanks,
Matthew

@juggernautsei
Contributor

The project management piece. We have not gotten far enough to be thinking
about "tasking out" parts yet. Although that was a suggestion, we should
also wait to hear back from free code camp to see what their response is.
Because I think it would be a better transition and quality control if we
can use free code camp. It would help take some work off of Brady's plate.

I would suggest using the community effort as a secondary option.

But the proof of concept is where you are working on now with the
version.php & etc.
​I read through the Doctrine tutorial but I took no action since I wanted
to see what you developed first before diving in myself. I don't want us to
develop to apposing path but be in unison so I am waiting to follow your
lead. I will help build on the proof of concept you lay out. ​

​Cheers​
S
​herwin

On Wed, Nov 16, 2016 at 8:03 AM, Matthew Vita notifications@github.com
wrote:

@juggernautsei https://github.com/juggernautsei now that we've
identified the areas that we'll need to change, we just have to get the
composer stuff in line. Let's read the materials @bradymiller
https://github.com/bradymiller just sent over as well as that article
about the basics of Doctrine (http://docs.doctrine-project.
org/projects/doctrine-orm/en/latest/tutorials/getting-started.html).

Also, I don't think we came to a conclusion about the project management
piece. Are you advocating that we "task out" everything in the Wiki and/or
Github Project Tasks in clear terms or do you want to "discover the needed
tasks as we go" as is the case now? I'm fine with either approach but I
will say I'm not convinced that the former provides a lot of value. I'm
interested in your thoughts.

Thanks,
Matthew


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#337 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAy9jtMniCEY34-IqYc8FkpLzHKDAyeEks5q-v8KgaJpZM4Kn-Kf
.

@juggernautsei
Contributor

​Mat,

I hope all is going well with you.
I have not heard from you in a few days,

I have been looking at the Zend work that ZH has placed in the code base.

Have you considered building on what they have started or is what your
planning a total departure from what is in placed i.e. the modules feature
that in place now.

Regard,
Sherwin​

On Wed, Nov 16, 2016 at 10:27 AM, Sherwin Gaddis sherwingaddis@gmail.com
wrote:

The project management piece. We have not gotten far enough to be thinking
about "tasking out" parts yet. Although that was a suggestion, we should
also wait to hear back from free code camp to see what their response is.
Because I think it would be a better transition and quality control if we
can use free code camp. It would help take some work off of Brady's plate.

I would suggest using the community effort as a secondary option.

But the proof of concept is where you are working on now with the
version.php & etc.
​I read through the Doctrine tutorial but I took no action since I wanted
to see what you developed first before diving in myself. I don't want us to
develop to apposing path but be in unison so I am waiting to follow your
lead. I will help build on the proof of concept you lay out. ​

​Cheers​
S
​herwin

On Wed, Nov 16, 2016 at 8:03 AM, Matthew Vita notifications@github.com
wrote:

@juggernautsei https://github.com/juggernautsei now that we've
identified the areas that we'll need to change, we just have to get the
composer stuff in line. Let's read the materials @bradymiller
https://github.com/bradymiller just sent over as well as that article
about the basics of Doctrine (http://docs.doctrine-project.
org/projects/doctrine-orm/en/latest/tutorials/getting-started.html).

Also, I don't think we came to a conclusion about the project management
piece. Are you advocating that we "task out" everything in the Wiki and/or
Github Project Tasks in clear terms or do you want to "discover the needed
tasks as we go" as is the case now? I'm fine with either approach but I
will say I'm not convinced that the former provides a lot of value. I'm
interested in your thoughts.

Thanks,
Matthew


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#337 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAy9jtMniCEY34-IqYc8FkpLzHKDAyeEks5q-v8KgaJpZM4Kn-Kf
.

@MatthewVita
Member

Hi Sherwin,

Yeah, I've been on the busy side.

I think we should use Doctrine in the existing areas of OpenEMR (which is the majority) as well as in the Zend area.

Ideally, OpenEMR will use Zend for everything, but that is far off.

Going to pick back up tonight with the version table work.

Thanks,
Matthew

@MatthewVita
Member

I'm struggling on getting wiring right. For instance, this is my entity:

/**
 * @ORM\Table(name="version")
 * @ORM\Entity
 */
class Version {
  /**
   * @var integer $v_major
   *
   * @ORM\Column(name="v_major", type="int", length=11, nullable=false, options={"default" : 0})
   */
  private $major;

  /**
   * @var integer $v_minor
   *
   * @ORM\Column(name="v_minor", type="int", length=11, nullable=false, options={"default" : 0}))
   */
  private $minor;

  /**
   * @var integer $v_patch
   *
   * @ORM\Column(name="v_patch", type="int", length=11, nullable=false, options={"default" : 0}))
   */
  private $patch;

  /**
   * @var integer $v_realpatch
   *
   * @ORM\Column(name="v_realpatch", type="int", length=11, nullable=false, options={"default" : 0}))
   */
  private $realPatch;

  /**
   * @var integer $v_tag
   *
   * @ORM\Column(name="v_tag", type="varchar", length=31, nullable=false, options={"default" : ""}))
   */
  private $tag;

  /**
   * @var integer $v_database
   *
   * @ORM\Column(name="v_database", type="int", length=11, nullable=false, options={"default" : 0}))
   */
  private $database;

  /**
   * @var integer $v_acl
   *
   * @ORM\Column(name="v_acl", type="int", length=11, nullable=false, options={"default" : 0}))
   */
  private $acl;

  public function getMajor() {
    return $this->major;
  }

  public function setMajor($value) {
    $this->major = $value;
  }

  public function getMinor() {
    return $this->minor;
  }

  public function setMinor($value) {
    $this->minor = $value;
  }

  public function getPatch() {
    return $this->patch;
  }

  public function setPatch($value) {
    $this->patch = $value;
  }

  public function getRealPatch() {
    return $this->realPatch;
  }

  public function setRealPatch($value) {
    $this->realPatch = $value;
  }

  public function getTag() {
    return $this->tag;
  }

  public function setTag($value) {
    $tag = $value;
  }

  public function getDatabase() {
    return $this->database;
  }

  public function setDatabase($value) {
    $this->database = $value;
  }

  public function getAcl() {
    return $this->acl;
  }

  public function setAcl($value) {
    $this->acl = $value;
  }
}

But I'm having a hard time with:

$em = $this->getServiceLocator()
           ->get('doctrine.entitymanager.orm_default');

$data = $em->getRepository('Version')->findAll();

foreach($data as $key => $row) {
  echo $row->getMajor();
}

I think I'm going to set up a basic composer PHP app just to get outside of OpenEMR to see what I'm doing wrong. I think it has something to do with the autoloader and the folder structure that Doctrine expects.

-m

@MatthewVita
Member

...and a couple of hours later, I have a semi-working proof-of-concept vanilla Doctrine/Composer project. Had to Stack Overflow/Google my way through :)

After work tomorrow, I will clean up, document, and zip up my code so we can begin to determine the best places to store the entities, repositories, etc in OpenEMR as it stands now (outside of Zend and within Zend).

-m

@kevinelong

All good things!

Please be sure to do performance testing if you want to support high volume
transactions.

Adding layers can simplify code tremendously but always at a cost in
performance.
Sometimes its as little as 10% sometimes its 10x depending on numerous
factors.

+1 for doctrine

We also use the light weight ORData.php which is not hirrible but can
easily be misused.

-Kevin Long

On Monday, November 21, 2016, Matthew Vita notifications@github.com wrote:

...and a couple of hours later, I have a semi-working proof-of-concept
vanilla Doctrine/Composer project. Had to Stack Overflow/Google my way
through :)

After work tomorrow, I will clean up, document, and zip up my code so we
can begin to determine the best places to store the entities, repositories,
etc in OpenEMR as it stands now (outside of Zend and within Zend).

-m


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#337 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AASYQZsWtOljf-kC3adCiqMYR5jvogTuks5rAna5gaJpZM4Kn-Kf
.

Kevin Ernest Long
Clever Clever Consulting Partners
www.cleverclever.com

503.888.6879
kevin@cleverclever.com
@kevinelong

@juggernautsei
Contributor

Well great job Mat!
I know the feeling of trying to get something to work and then the eureka
moment.

I was looking at what you have posted and I just had one question.

Had you considered taking advantage of auto instantiation?

Kevin is right about the ORData.php being light weight. I think it's
original intent was to take advantage of the polymorphism in it's early
days.
It took me a while to figure out why it was there but as my knowledge grow
that is my guess as to it's purpose of being in most of the older class
files.

I am excited to see where this is going for the future of the whole
project.

Sherwin
757-328-2736
Open Med Practice

   www.openmedpractice.com

OpenEMR Simplicity at its best in cloud based EMR's

“For those who believe, no proof is necessary. For those who don't believe,
no proof is possible http://beingsaved.org/innercoc.va.”

On Tue, Nov 22, 2016 at 12:12 AM, Kevin Ernest Long <
notifications@github.com> wrote:

All good things!

Please be sure to do performance testing if you want to support high volume
transactions.

Adding layers can simplify code tremendously but always at a cost in
performance.
Sometimes its as little as 10% sometimes its 10x depending on numerous
factors.

+1 for doctrine

We also use the light weight ORData.php which is not hirrible but can
easily be misused.

-Kevin Long

On Monday, November 21, 2016, Matthew Vita notifications@github.com
wrote:

...and a couple of hours later, I have a semi-working proof-of-concept
vanilla Doctrine/Composer project. Had to Stack Overflow/Google my way
through :)

After work tomorrow, I will clean up, document, and zip up my code so we
can begin to determine the best places to store the entities,
repositories,
etc in OpenEMR as it stands now (outside of Zend and within Zend).

-m


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#337 (comment),
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AASYQZsWtOljf-
kC3adCiqMYR5jvogTuks5rAna5gaJpZM4Kn-Kf>
.

Kevin Ernest Long
Clever Clever Consulting Partners
www.cleverclever.com

503.888.6879
kevin@cleverclever.com
@kevinelong


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#337 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAy9jkWCIh_J4DEhLjBLV2uITKb9hheTks5rAnmvgaJpZM4Kn-Kf
.

@MatthewVita
Member

@kevinelong, Thanks for the feedback. I am experienced with Hibernate (Java) and optimization in terms of when to rely on classic annotations vs just using the special DSL query language to ensure a better query and fetching strategies. Performance is super important for OpenEMR, obviously (e.g.: watching out for N+1 rundancy selects, etc).

Would you be interested in working with Sherwin and I on this project? It's really just getting started and I have a feeling that we'll be reaching a point of momentum here shortly.

@juggernautsei Exciting stuff! In terms of auto-instantiation, do you mean composer autoloading or eager fetching? Are you talking about the entities themselves leveraging some sort of special builder pattern?

Also, I think that once we get a few tables mapped out (esp those with more complex JOINs), we'll have a lot of momentum. Swapping out the current queries in OpenEMR to use our new repositories and entities will be relatively trivial. However, this will be bad in terms in best practices because we'll be swapping out the bad SQL in views and controllers, but that will be addressed in another ticket (MVC switch over). Obviously database stuff should be dealt with at the service layer.

Will have some code for you to check out after work today.

PS: Your website is very well designed.

-m

@MatthewVita
Member

Okay, here's my sample application using just Composer/Doctrine:

composer.json (probably not following best practices with the * here):

{
    "name": "matthewvita/doctrine_example",
    "authors": [
        {
            "name": "Matthew Vita",
            "email": "matthewvita@live.com"
        }
    ],
    "require": {
       "doctrine/orm": "*"
     },
    "autoload": {
        "psr-0": {
            "App": "App/"
        },
        "classmap": ["App", "App/Logger", "App/Database", "App/Entity", "App/Respository"]
    }
}

bootstrap.php:

<?php
require_once "vendor/autoload.php";

$application = new App\Application();
$application->runDoctrineTests();

App/Database/Connector.php:

<?php
namespace App\Database;

use Doctrine\ORM\Tools\Setup;
use Doctrine\ORM\EntityManager;

class Connector {
  public $entityManager;

  public function __construct() {
    $entityPath = array("App/Entity");

    // TODO: bring in these values via configuration
    $isDevMode = false;
    $connection = array(
      'driver'   => 'pdo_mysql',
      'user'     => 'super_secret_user_from_config',
      'password' => 'super_secret_password_from_config',
      'dbname'   => 'super_secret_database_from_config'
    );

    $annotationConfiguration = Setup::createAnnotationMetadataConfiguration($entityPath, $isDevMode, null, null, false);

    $this->entityManager = EntityManager::create($connection, $annotationConfiguration);
  }
}

App/Application.php:

<?php
namespace App;

class Application {
  private $entityManager;
  private $logger;

  public function __construct() {
    $this->logger = new Logger\Logger("Application");
    $database = new Database\Connector();
    $this->entityManager = $database->entityManager;
  }

  public function runDoctrineTests() {
    $this->testRetrievalOfInitialVersion();
    $this->testCreationOfVersion();
    $this->testUpdatingOfVersion();
    $this->testDeletionOfVersion();
  }

  private function testRetrievalOfInitialVersion() {
    $this->logger->debug("Testing the retrieval of the initial version");

    $version = $this->entityManager->find('App\Entity\Version', 195); // Database is primary id

    if ($version != null) {
      $this->logger->debug("Version found with properties " . $version);
    } else {
      $this->logger->warn("Version not found");
    }
  }

  private function testCreationOfVersion() {
    $this->logger->debug("Testing the creation of the version 6");

    try {
      $version = new Entity\Version();

      $version->setAcl(3);
      $version->setDatabase(9001);
      $version->setTag("-dev");
      $version->setRealPatch(0);
      $version->setPatch(0);
      $version->setMinor(0);
      $version->setMajor(6);
      $this->entityManager->persist($version);
      $this->entityManager->flush();

      $this->logger->debug("Version 6 created");
    } catch (PDOException $exception) {
      $this->logger->warn("Version 6 not created due to error " . $exception->getMessage());
    }
  }

  private function testUpdatingOfVersion() {
    $this->logger->debug("Testing the updating of version 6");

    try {
      $version = $this->entityManager->find('App\Entity\Version', 9001);

      $version->setMajor(6);
      $this->entityManager->persist($version);
      $this->entityManager->flush();

      $this->logger->debug("Version 6 updated");
    } catch (PDOException $exception) {
      $this->logger->warn("Version 6 not updated due to error " . $exception->getMessage());
    }
  }

  private function testDeletionOfVersion() {
    $this->logger->debug("Testing the deletion of version 6");

    try {
      $version = $this->entityManager->find('App\Entity\Version', 9001);

      $this->entityManager->remove($version);
      $this->entityManager->flush();

      $this->logger->debug("Version 6 deleted");
    } catch (PDOException $exception) {
      $this->logger->warn("Version 6 not deleted due to error " . $exception->getMessage());
    }
  }
}

App/Repository/VersionRepository.php:

<?php

namespace App\Respository;

use App\Entity\Version;
use Doctrine\ORM\EntityManager;

class VersionRespository {
  /**
   * @var string
   */
  private $class = 'App\Entity\Version';

  /**
   * @var EntityManager
   */
  private $entityManager;

  public function __construct(EntityManager $entityManager) {
    $this->entityManager = $entityManager;
  }

  public function create(Version $version) {
    $this->entityManager->persist($version);
    $this->entityManager->flush();
  }

  public function update(Version $version, $data) {
    $this->entityManager->persist($post);
    $this->entityManager->flush();
  }

  public function delete(Version $version) {
    $this->entityManager->remove($version);
    $this->entityManager->flush();
  }

  /**
   * create Version
   * @return Version
   */
  private function prepareData($data) {
    return new Version($data);
  }
}

App/Entity/Version.php:

<?php
namespace App\Entity;

use Doctrine\ORM\Mapping\Entity;
use Doctrine\ORM\Mapping\Table;
use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\Id;

/**
 * @Table(name="version")
 * @Entity
 * @Entity(repositoryClass="App\Repository\VersionRepository")
 */
class Version {
  public function __construct() {}

  /**
   * @var integer $v_major
   *
   * @Column(name="v_major", type="integer", length=11, nullable=false, options={"default" : 0})
   */
  private $major;

  /**
   * @var integer $v_minor
   *
   * @Column(name="v_minor", type="integer", length=11, nullable=false, options={"default" : 0}))
   */
  private $minor;

  /**
   * @var integer $v_patch
   *
   * @Column(name="v_patch", type="integer", length=11, nullable=false, options={"default" : 0}))
   */
  private $patch;

  /**
   * @var integer $v_realpatch
   *
   * @Column(name="v_realpatch", type="integer", length=11, nullable=false, options={"default" : 0}))
   */
  private $realPatch;

  /**
   * @var integer $v_tag
   *
   * @Column(name="v_tag", type="string", length=31, nullable=false, options={"default" : ""}))
   */
  private $tag;

  /**
   * @var integer $v_database
   *
   * @Id
   * @Column(name="v_database", type="integer", length=11, nullable=false, options={"default" : 0}))
   */
  private $database;

  /**
   * @var integer $v_acl
   *
   * @Column(name="v_acl", type="integer", length=11, nullable=false, options={"default" : 0}))
   */
  private $acl;

  public function getMajor() {
    return $this->major;
  }

  public function setMajor($value) {
    $this->major = $value;
  }

  public function getMinor() {
    return $this->minor;
  }

  public function setMinor($value) {
    $this->minor = $value;
  }

  public function getPatch() {
    return $this->patch;
  }

  public function setPatch($value) {
    $this->patch = $value;
  }

  public function getRealPatch() {
    return $this->realPatch;
  }

  public function setRealPatch($value) {
    $this->realPatch = $value;
  }

  public function getTag() {
    return $this->tag;
  }

  public function setTag($value) {
    $this->tag = $value;
  }

  public function getDatabase() {
    return $this->database;
  }

  public function setDatabase($value) {
    $this->database = $value;
  }

  public function getAcl() {
    return $this->acl;
  }

  public function setAcl($value) {
    $this->acl = $value;
  }

  public function __toString() {
    return "acl: '" . $this->getAcl() . "' " .
           "database: '" . $this->getDatabase() . "' " .
           "tag: '" . $this->getTag() . "' " .
           "realPatch: '" . $this->getRealPatch() . "' " .
           "patch: '" . $this->getPatch() . "' " .
           "minor: '" . $this->getMinor() . "' " .
           "major: '" . $this->getMajor() . "'";
  }
}

App/Logger/Logger.php:

<?php
namespace App\Logger;

// Note: very simple logger for testing. Doesn't roll logs or anything.
class Logger {
  private $classContext;
  private $logFile = "application.log";

  public function __construct($classContext) {
    $this->classContext = $classContext;
  }

  public function info($message) {
    $this->log($message, "INFO");
  }

  public function debug($message) {
    $this->log($message, "DEBUG");
  }

  public function warn($message) {
    $this->log($message, "WARN");
  }

  public function error($message) {
    $this->log($message, "ERROR");
  }

  private function log($message, $type) {
    $logEntry = date("Y-m-d H:i:s") . " [" . $type . "] " . $this->classContext . " - " . $message;

    echo($logEntry . "\n");
    file_put_contents($this->logFile, $logEntry.PHP_EOL , FILE_APPEND | LOCK_EX);
  }
}

...and when I run > composer update && php bootstrap.php, here is application.log:

2016-11-22 21:21:58 [DEBUG] Application - Testing the retrieval of the initial version
2016-11-22 21:21:58 [DEBUG] Application - Version found with properties acl: '3' database: '195' tag: '-dev' realPatch: '0' patch: '0' minor: '0' major: '5'
2016-11-22 21:21:58 [DEBUG] Application - Testing the creation of the version 6
2016-11-22 21:21:58 [DEBUG] Application - Version 6 created
2016-11-22 21:21:58 [DEBUG] Application - Testing the updating of version 6
2016-11-22 21:21:58 [DEBUG] Application - Version 6 updated
2016-11-22 21:21:58 [DEBUG] Application - Testing the deletion of version 6
2016-11-22 21:21:58 [DEBUG] Application - Version 6 deleted
@MatthewVita
Member

A few notes:

  • @juggernautsei Let's use the above as inspiration to wire this up in OpenEMR. We should both work on this until one of us figures it out first (just post the solution in a work-in-progress pull request when ready). Recall that the following files will need tweaked:
    • openemr/version.php
    • openemr/admin.php
    • openemr/sql_upgrade.php
    • openemr/library/classes/Installer.class.php
    • openemr/sql_patch.php
  • PDOException isn't actually getting handled in the catch block. Not sure why :/.
  • We need to think about switch the database connector piece to a) be a singleton and b) use a db pool
  • Here is version 6 __toString() just because I didn't include it above:
    2016-11-22 21:24:08 [DEBUG] Application - acl: '3' database: '9001' tag: '-dev' realPatch: '0' patch: '0' minor: '0' major: '6'

...ORM goodness!

@juggernautsei
Contributor

Mat,

I will take what you have done and toy with it to see if I can get it
working on my dev and then report back to you.
Great work!

If you could stick a few notes in the code to say what this and that are
for it would make the road map easier to follow.
Rod is about the only person that does a lot of notations. I hope going
forward the project can be notated better. It helps to cut down the
learning curve and guess work.

Appreciate you!

On Tue, Nov 22, 2016 at 9:30 PM, Matthew Vita notifications@github.com
wrote:

A few notes:

  • @juggernautsei https://github.com/juggernautsei Let's use the
    above as inspiration to wire this up in OpenEMR. We should both work on
    this until one of us figures it out first (just post the solution in a
    work-in-progress pull request when ready). Recall that the following files
    will need tweaked:
    • openemr/version.php
    • openemr/admin.php
    • openemr/sql_upgrade.php
    • openemr/library/classes/Installer.class.php
    • openemr/sql_patch.php
  • PDOException isn't actually getting handled in the catch block. Not
    sure why :/.
  • We need to think about switch the database connector piece to a) be
    a singleton and b) use a db pool
  • Here is version 6 __toString() just because I didn't include it
    above:
    2016-11-22 21:24:08 [DEBUG] Application - acl: '3' database: '9001'
    tag: '-dev' realPatch: '0' patch: '0' minor: '0' major: '6'

...ORM goodness!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#337 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAy9jp1cbzNW0PEMMKsu_DAP5legA1iXks5rA6VMgaJpZM4Kn-Kf
.

@MatthewVita
Member

@juggernautsei,

Here's a Work-In-Progress PR I put together for version: 0186021 (please note there is another commit on this branch that has the composer resources... this commit is just my code changes for ease of reviewing).

I agree with you that documentation is very important. You'll find docs that are loosely based on PHPDoc in my PR.

Please review my PR and provide feedback.

Next step is actually hooking up code that touches the version table to use Doctrine.

Thanks,
Matthew

@MatthewVita
Member
MatthewVita commented Nov 29, 2016 edited

@juggernautsei: Sorry for my silence the past few days (family emergency). Have you had a chance to check out the PR? I will be back working on this soon.

@bradymiller: Thanks for the code review feedback. Will address that.

@juggernautsei
Contributor

Mat,
I have kind of been in a tail spin myself for the past week looking for work and work that has kept me out of the development chair. I will focus on this project in the coming week.

@juggernautsei
Contributor

@MatthewVita : have you been able to work on this project of late?

@MatthewVita
Member
MatthewVita commented Dec 10, 2016 edited

Hi @juggernautsei, my wife is now out of the hospital and things are going back to normal. I am looking forward to picking back up with this project as early as tomorrow! Here's my immediate task list:

  • Implement the code review suggestions from @bradymiller
  • Implement the version ORM interactions in the current code base
  • Identify the next entity we should map out (preferably one that is a bit more challenging... e.g.: involves JOINs)

Can you look into setting up database connection pooling with Doctrine? I put this on my branch but I'm not sure if has any effect: MatthewVita@0186021#diff-ba0eab5d5560e061a23274e580fcc366R97

Thanks,
Matthew

@juggernautsei
Contributor

Hi Matt, glad to hear that your wife is doing well.

I finally got around to looking at your code and loading the Doctrine getting started project on my dev server. I downloaded and setup the code that you have posted.

I find the doctrine 2 tutorials light in real instruction. They are covered with theory but lack the instruction to build a meaningful first project.
I did come across a tutorial that got me closer to a meaningful project. I am still trying to work in all the details. I have to figure out how
to access the data and pass data back and forth before I start to unwind what you have created.

I do have the focus in mind about the data persistence. I will get back with you in a couple of days to inform you of my progress.

Sherwin

@MatthewVita
Member

Hi @juggernautsei,

Thanks for your patience.

I agree that Doctrine 2 tutorials from 3rd parties are a bit more practical. Let me know if you have any questions along the way of your learning Doctrine.

I'm working on 2 things:

Thanks,
Matthew

@juggernautsei
Contributor

I would like to talk this out to see if I am wrapping my head around what work you have done and what I think I understand.
I am looking at the version_repository.php file and I noticed that there is a basic CRUD system in place but the Read is missing. (I'm still playing catch up). Why is there no list all function or is that not needed.

I found an article
http://symfony.com/doc/current/bundles/SensioGeneratorBundle/commands/generate_doctrine_crud.html

Have you seen something like this?

I'm assuming that the consumers would be other developers that wish to consume the data in the portion of the application they are building?

Next question, this file is not being used anywhere in the system. Is that correct?

@MatthewVita
Member
MatthewVita commented Dec 14, 2016 edited

Hi @juggernautsei,

I am looking at the version_repository.php file and I noticed that there is a basic CRUD system in place but the Read is missing. (I'm still playing catch up). Why is there no list all function or is that not needed.

Note that our class VersionRepository extends a Doctrine base class called "EntityRepository". This base class gives you the following methods for "free":

  • find(mixed $id, integer|null $lockMode = null, integer|null $lockVersion = null) Finds an entity by its primary key / identifier.
  • findAll() Finds all entities in the repository.
  • findBy(array $criteria, array $orderBy = null, integer|null $limit = null, integer|null $offset = null) Finds entities by a set of criteria.
  • findOneBy(array $criteria, array $orderBy = null) Finds a single entity by a set of criteria.

However, delete(), create(), etc have to be implemented by us (Doctrine doesn't provide those methods). You can mix in special methods in this area as well... for instance, a getAllVersionsThatStartWithAnOddNumber() method can be added to the VersionRepository.

Here's how the entity is used in conjunction with the repository:

<?php
 try {
      $database = \common\database\Connector::Instance(); //  uses singleton pattern
      $entityManager = $database->entityManager;
       
      // In doctrine, repositories and entities are coupled, so one must specify the entity to get the relevant repository.
      $repository = $entityManager->getRepository('\entities\Version');

      $version = new \entities\Version(); // object to be stored in db
      $version->setAcl(3);
      $version->setDatabase(9001);
      $version->setTag("-dev");
      $version->setRealPatch(0);
      $version->setPatch(0);
      $version->setMinor(0);
      $version->setMajor(7); //Version 7, woah!
      $repository->create($version);
    } catch (PDOException $exception) {
      // uh oh logic here
    }
?>

I found an article http://symfony.com/doc/current/bundles/SensioGeneratorBundle/commands/generate_doctrine_crud.html Have you seen something like this?

This is Symfony-specific so we won't get much use out of such a CLI or the conventions that are enforced by Symfony.

I'm assuming that the consumers would be other developers that wish to consume the data in the portion of the application they are building?

Yeah, ORM abstraction should be used everywhere a db interaction happens. The best place for them is in the service layer. Will take a while to modernize all of the current code to follow this best practice, however.

Next question, this file is not being used anywhere in the system. Is that correct?

Not yet. However, I addressed @bradymiller's code review feedback and will be getting this entity in place hopefully tomorrow. I also need to rebase the branch and update composer,.

Thanks,
Matthew

@MatthewVita
Member

@juggernautsei just checked in the auditing piece to my branch #357

I've updated the PR with a TODO list here: #357 (comment)

@juggernautsei
Contributor

@MatthewVita
I have time to catch up.
Where can I find the repository of all this put together?
I tried following the pull requests but could not find your base repository.

@MatthewVita
Member

@juggernautsei clone https://github.com/MatthewVita/openemr.git and checkout doctrine_and_other_moderization_work

:)

@MatthewVita
Member

here's the TODO list: #357 (comment)

@juggernautsei
Contributor

@MatthewVita
I have the code you have on git loaded and I ran a search for "autoload": { because I wanted to see if I could find the work that you have posted here. I didn't. I am blindly looking around. Help.

@MatthewVita
Member

Not sure what you mean by autoloading. If you're looking for the code files, here they are:

to actually use it, you can put this test code anywhere and run it:

<?php
 try {
      $database = \common\database\Connector::Instance(); //  uses singleton pattern
      $entityManager = $database->entityManager;
       
      // In doctrine, repositories and entities are coupled, so one must specify the entity to get the relevant repository.
      $repository = $entityManager->getRepository('\entities\Version');

      $version = new \entities\Version(); // object to be stored in db
      $version->setAcl(3);
      $version->setDatabase(9001);
      $version->setTag("-dev");
      $version->setRealPatch(0);
      $version->setPatch(0);
      $version->setMinor(0);
      $version->setMajor(7); //Version 7, woah!
      $repository->create($version);
    } catch (PDOException $exception) {
      // uh oh logic here
    }
?>
@MatthewVita
Member
MatthewVita commented Jan 7, 2017 edited

Btw, I'll have an updated branch/PR at some point tonight with:

  • rebase
  • improvements to the application logger
  • @bradymiller's excellent composer work

Thanks,
Matthew

EDIT: the above work has been completed. I follow Brady's composer update guide and I believe I did it correctly! I deleted the TODO list on the PR because the only items now are to integrate the version entity in the codebase (will do that today or tomorrow... pretty trivial) and get the code merged in so we can start working on more interesting models (ones with JOINs, for instance).

@juggernautsei
Contributor

Matthew,
with great anticipation I copied all the files you had listed to the respective folders that you so graciously listed. I made a folder called \versioning in the root folder, then saved the test code to that folder.
I launched the test code and no joy the code does not update the table data. I tried chasing down the problem but there are no PHP errors in the error log. I commented everything in the try block but the first line and the first line failed to load. Any suggestions?

@MatthewVita
Member
MatthewVita commented Jan 8, 2017 edited

Hi @juggernautsei,

Sorry to hear you're running into a speed bump.

I copied all the files you had

Did you clone my branch before copying the files over? If you haven't, you'll need to a) just clone the branch and things will work or b) also copy the composer.json and interface/globals.php files from my branch to your local OpenEMR instance. I didn't list those before because I figured you had cloned my branch.

Thanks,
Matthew

@MatthewVita
Member
MatthewVita commented Jan 8, 2017 edited

Service Implementation Updates:

Note these are the files that need to interact with the VersionService (1), which relies on the VersionRepository (2):

  • admin.php
  • library/classes/Installer.class.php
  • sql_upgrade.php
  • sql_patch.php

(1) services/version_service.php

<?php
/**
 * VersionService
 *
 * LICENSE: This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
 * GNU General Public License for more details.
 * You should have received a copy of the GNU General Public License
 * along with this program. If not, see <http://opensource.org/licenses/gpl-license.php>;.
 *
 * @package OpenEMR
 * @author  Matthew Vita <matthewvita48@gmail.com>
 * @link    http://www.open-emr.org
 */

namespace services;

use entities\Version;

class VersionService {
    private $logger;

    private $repository;

    public function __construct() {
        $this->logger = new \common\logging\Logger("\services\VersionService");
        $database = \common\database\Connector::Instance();
        $entityManager = $database->entityManager;
        $this->repository = $entityManager->getRepository('\entities\Version');
    }

    public function doesTableExist() {
        $this->logger->debug("Checking to see if the version table exists");
        return $this->repository->doesTableExist();
    }

    public function fetch() {
        $this->logger->debug("Fetching the version entry");
        $version = $this->repository->findFirst();

        if (empty($version)) {
          $this->logger->error("No version found");
          return null;
        }

        return $version;
    }

    public function update(Version $version) {
        $this->logger->debug("Updating version entry");
        if (!$this->canRealPatchBeApplied($version)) {
          $version->setRealPatch(0);
        }

        return $this->repository->update($version);
    }

    public function canRealPatchBeApplied(Version $version) {
        $this->logger->debug("Determining if a real patch can be applied");
        return !empty($version->getRealPatch()) && ($version->getRealPatch() != "") && ($version->getRealPatch() > 0);
    }
}

(2) repositories/version_repository.php

<?php
/**
 * Version repository.

 * Copyright (C) 2016 Matthew Vita <matthewvita48@gmail.com>
 *
 * LICENSE: This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
 * GNU General Public License for more details.
 * You should have received a copy of the GNU General Public License
 * along with this program. If not, see <http://opensource.org/licenses/gpl-license.php>;.
 *
 * @package OpenEMR
 * @author  Matthew Vita <matthewvita48@gmail.com>
 * @link    http://www.open-emr.org
 */

namespace repositories;

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

class VersionRepository extends EntityRepository {
    public function update(Version $version) {
        $objectToBeUpdated = $this->updateNonKeyedEntityObject($this->findFirst(), $version);
        $updateInformation = $this->_em->persist($objectToBeUpdated);
        $this->_em->flush();

        return $updateInformation;
    }

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

        return null;
    }

    public function doesTableExist() {
        $query = $this->_em->getConnection()->prepare("SHOW TABLES LIKE 'version'");
        $query->execute();
        $results = $query->fetch();

        return !empty($results);
    }

    private function updateNonKeyedEntityObject(Version $objectToBeUpdated, Version $newObject) {
        if (!empty($objectToBeUpdated)) {
            $objectToBeUpdated->setAcl($newObject->getAcl());
            $objectToBeUpdated->setDatabase($newObject->getDatabase());
            $objectToBeUpdated->setTag($newObject->getTag());
            $objectToBeUpdated->setRealPatch($newObject->getRealPatch());
            $objectToBeUpdated->setPatch($newObject->getPatch());
            $objectToBeUpdated->setMinor($newObject->getMinor());
            $objectToBeUpdated->setMajor($newObject->getMajor());

            return $objectToBeUpdated;
        }

        return null;
    }
}

Please note that:

  • Not using a controllers/version_controller.php yet so the controller logic will live in the view for now ( @juggernautsei doing this would require a good amount of work and testing. Do you want to put this work off until all of the Doctrine work is done or do you want to implement the C in MVC as we make progress with Doctrine? I'm leaning towards let's get Doctrine done first but I want to know your thoughts.)
  • Because there are no unique keys with the version table, I had to implement the hacky updateNonKeyedEntityObject method so that Doctrine wouldn't simply INSERT a new entity because there is no unique row match.
  • I still need to do the documentation (haven't pushed yet because of this) (done, just pushed to #357)

Big Question for @bradymiller

I can't new \services\VersionService() in admin.php (assuming in the other 3 files as well. How do I get our composer project to respect autoloading in these areas?

Update: I can now bring in the VersionServce via autoload, but I have to reference globals.php and rename the local sqlQuery function to _sqlQuery. I'm not sure this is a great idea, but at least I can continue my work as we talk about the implications. I am including a screenshot so you can see the lines that were adding to this file (git gutter puts + in the line gutter to indicate deltas):

image

Thanks,
Matthew

@MatthewVita
Member
MatthewVita commented Jan 8, 2017 edited

oops, it should be $ignoreAuth=false obviously

UPDATE: Actually I'm seeing $ignoreAuth = true; // no login required in the sql_upgrade.php and sql_patch.php files. Is this intentional?

@MatthewVita
Member
MatthewVita commented Jan 9, 2017 edited

@bradymiller Everything is now pushed (including PSR-4 which appears to be magically working). Can you run through the tests that I mentioned in Slack?

@juggernautsei So it turns out that admin.php and library/classes/Installer.class.php are not going to be able to use the new Doctrine code because they are outside of OpenEMR really (they lack application context). The files that were able to be changed to use the new Doctrine stuff were sql_upgrade.php, sql_patch.php and globals.php which affects interface/main/about_page.php. Please review and run the code.

An aside: I'm going to start thinking about which tables we should convert to Doctrine next. Now that we've learned that anything "outside" is going to be a real pain, we should stick to tables that we know are only used internally. Moreover, I'd really like to make a repository that handles JOINs. Once the tables are identified, I will write down a list either as Github tickets or on the wiki.

@juggernautsei
Contributor

I downloaded the updated clone of your repository and updated what I had on my system. I attached a screenshot of the code that I am attempting to run in the codebase.

I checked the common folder and it was there and the files that you pointed out where there. I used the same folder I created and attempted to run the code .

doctrinem

It still does not run. I am using a windows server 2008 with WAMP 3.0.6 PHP 7 Apache 2.4.23 MYSQL 5.7.14.

I don't get hard fail. I still get no error messages but It will not run. http://omp.openmedpractice.com/doctrineM/test_version/versioning.php
And if you are questioning do I log in first I do with admin and pass.

doctrinem-2

I like to use print/echo statement for troubleshooting how far the code is running. The code does not get to the second print statement. Your thoughts?

Yes these are: Actually I'm seeing $ignoreAuth = true; // no login required in the sql_upgrade.php and sql_patch.php files. Is this intentional?
As you pointed out they are outside of the context of OpenEMR so that login is not required.

To the question of controller class, it would be wise to wait till the doctrine feature is stable before moving to the controller. Working on many fronts is not wise.

@MatthewVita
Member

Hi Sherwin,

You may need to pull again because I updated the branch last night once more (code is now PSR-4 compliant).

As you pointed out they are outside of the context of OpenEMR so that login is not required.

This may be the issue. PSR-4 autoloading won't work unless the code is ran inside of the OpenEMR context. For example, if you put that code (randomly) in login.php, it will probably work.

To the question of controller class, it would be wise to wait till the doctrine feature is stable before moving to the controller. Working on many fronts is not wise.

I agree.

I am going to set up a Windows OpenEMR with WAMP so I can start testing on both *nix and Windows moving forward. I will update you on any interesting finds there.

Thanks,
Matthew

@juggernautsei
Contributor

Unhappy to report that I pulled the code again and tried with the same not functional result.
I have a Linux server I can run it on. I choose to develop on Windows.

@MatthewVita
Member
@juggernautsei
Contributor

Just so we are on the same page. Do you just want me to wrap the code in a class or is there a particular class you want me to put the code in?

@juggernautsei
Contributor

Is this what you mean?

class version {

public function __construct(){

     $database = \common\database\Connector::Instance(); //  uses singleton pattern
     $entityManager = $database->entityManager;
  
 
      // In doctrine, repositories and entities are coupled, so one must specify the entity to get the relevant repository.
      $repository = $entityManager->getRepository('\entities\Version');

      $version = new \entities\Version(); // object to be stored in db

      $version->setAcl(3);
      $version->setDatabase(9001);
      $version->setTag("--dev");
      $version->setRealPatch(0);
      $version->setPatch(0);
      $version->setMinor(2);
      $version->setMajor(7); //Version 7, woah!
      $repository->create($version);
     } 

}
$v = new version;

@juggernautsei
Contributor
juggernautsei commented Jan 10, 2017 edited

I fixed my code still waiting to hear back from you.
`
<?php

       class VersionUpdater {


        public function newVersion(){

     $database = \common\database\Connector::Instance(); //  uses singleton pattern
     $entityManager = $database->entityManager;
  
 
      // In doctrine, repositories and entities are coupled, so one must specify the entity to get the relevant repository.
      $repository = $entityManager->getRepository('\entities\Version');

      $version = new \entities\Version(); // object to be stored in db

      $version->setAcl(3);
      $version->setDatabase(9001);
      $version->setTag("--dev");
      $version->setRealPatch(0);
      $version->setPatch(0);
      $version->setMinor(2);
      $version->setMajor(7); //Version 7, woah!
      $repository->create($version);

      return "Done";
      
     } 

     

}
    $v = new VersionUpdater();
    echo $v->newVersion;
    ?>

`
Still not working as far as I can tell.

@MatthewVita
Member
MatthewVita commented Jan 11, 2017 edited

Do you just want me to wrap the code in a class or is there a particular class you want me to put the code in?

I was using the term "class" generically. What I was really getting at is put the Doctrine interaction code inside any OpenEMR php file that is within the OpenEMR context. For example, sneak the above code in https://github.com/MatthewVita/openemr/blob/doctrine_and_other_moderization_work/interface/login/login.php and visit that page. Although this is a silly example, it will work because the Doctrine can only be autoloaded in the OpenEMR context (interface/globals.php brings it in).

still waiting to hear back from you.

Was at day job.

Thanks,
Matthew

@MatthewVita
Member

By the way, I'm setting up my Windows OpenEMR development VM now :)

@MatthewVita
Member

@juggernautsei,

Good news, I got the Windows OpenEMR dev environment setup and sql_upgrade.php which uses the version_service.php works.

Please note that the code you are going to test with above will error out because I removed the create method from the version_repository.php because we aren't creating versions in the scope of the version service. You'll have to add the function in yourself. Off the top of my head it should be something like:

public function creation(Version $v) {
    $this->_em->persist($v);
    $this->_em->flush();
}

Thanks,
Matthew

@juggernautsei
Contributor

I stuck the code in the about file

about

It halts the execution of the file.

@MatthewVita
Member
MatthewVita commented Jan 11, 2017 edited

right, because I forgot that I removed the create function from the repository. You'll have to put that code in that I mentioned above.

We're not creating versions so it shouldn't go in there.

Sorry for the confusion. Had the function in there to begin with and used it for testing, then I realized that the .sql file that OpenEMR uses creates the version table, insert exactly one row, and all that will ever happen to that table later is the updating of that one row. There is never a need for inserting.

@MatthewVita
Member

In other good news, the new application loggers works/looks just fine on Windows:

image

@MatthewVita
Member

@juggernautsei here's the exact code you need to add to the version repository to see the creation piece work:

    /**
     * Create.
     *
     * @param the new Version entity.
     */
    public function create(Version $version) {
        $this->_em->persist($version);
        $this->_em->flush();
    }
@juggernautsei
Contributor

In my copy of the program that code is in the \repositories\version_repository.php file

Where are you getting the log data from? I can't tell from your screenshot.

@MatthewVita
Member

In my copy of the program that code is in the \repositories\version_repository.php file

Hmm, that tells me you need to reclone my branch because that was definitely removed (see https://github.com/MatthewVita/openemr/blob/5a71c3909f846fa208ac501566f09bb2a0d65a2b/repositories/version_repository.php). The code you have now is probably bailing out due to recent changes that I had to put in so that the sqlconf.php file wasn't affected.

Please reclone the branch and feel free to post any relevant bits from the Apache error.log.

Where are you getting the log data from? I can't tell from your screenshot.

This is the new OpenEMR application logger. The logs are stored in $/openemr/logs where ($ is where you installed it on your machine). Of course, Apache's error.log is the best for seeing general PHP errors, and OpenEMR's clever SQL auditor is essential for data updates, but this logger is for general application logging that will be introduced as we continue the modernization effort.

@MatthewVita
Member

Time for bed, talk to you tomorrow. I hope that re-pulling the latest version of the branch works for you. You'll still have to copy in that create method we discussed for additional testing... also please test the actual usage of the version_service in the sql_patch and sql_upgrade areas if you a change.

Thanks,
Matthew

@juggernautsei
Contributor

That is the weird thing is that I am getting no errors in the log files. Nothing at all related to the doctrine not running. I will clone again and see what I get this time.
Good night.

@MatthewVita
Member

We will figure this out and start to see a whole lot of momentum soon!

@juggernautsei
Contributor

openemr-5a71c3909f846fa208ac501566f09bb2a0d65a2b is the code that I now have installed and trying to execute the code requested . Is this the latest copy of the code you are using?

It still errors out.
http://omp.openmedpractice.com/doctrineM/test_version/versioning.php

Nothing in the php or apache error logs.

@MatthewVita
Member

Okay, that is the latest hash.

This is very strange. I'm not sure why it's erroring out with no logs. I'm verifying this is working on both Windows and Linux :/

You did put the create method in the repository, right?

@bradymiller can you test out this code to see if it works for you ?

Thanks,
Matthew

@bradymiller
Member

Tested this on Mint 18 (php7) and it appears to work (tested about, sql_upgrade, and sql_patch) and seemed to do right thing when increment versions and seeing the debug messages in the log file (which is really neat btw for development).

@juggernautsei
Contributor
juggernautsei commented Jan 12, 2017 edited

I added the create method and it does create the entries in the version table when I run it from the about and from the sql_upgrade. It still errors out if it is outside of the OpenEMR class.
Which it should right?

Great work!

@MatthewVita
Member
MatthewVita commented Jan 13, 2017 edited

@bradymiller

Tested this on Mint 18 (php7)

Thanks for testing. I also use Linux Mint!

seeing the debug messages in the log file (which is really neat btw for development).

Yeah, I've never worked on a codebase without application logging. I believe as the log statements increase with the modernization project, they will become more useful.

@juggernautsei

Which it should right?

Right, the autoloader is no where to be found.

Great work!

You too. Which entities do you want to model next? I was thinking it would be good to work with tables that need JOINs so we can dive deeper into Doctrine.

Thanks,
Matthew

@bradymiller
Member

@MatthewVita , @juggernautsei
If you need a really easy one to work on, onotes(office notes) would likely be a good one:
https://github.com/openemr/openemr/search?utf8=%E2%9C%93&q=onotes

@MatthewVita
Member

Okay, awesome!

I believe that this ticket can be closed [0] because it seems like the answer is "yes" for "Determine if using an ORM is appropriate". This means we can add the ticket for "Office Notes ORM Implementation" to either a new github ticket in the modernization project or to a fresh wiki page for project tracking. @juggernautsei how would you like to go about this?

[0] Well, I'd like to leave it open until the following items are tackled that came out of the code review (going to tackle them after work tomorrow):

  • Allow auditor to skip logs in certain contexts
  • Respect UTF-8 mode config
  • Respect strict SQL mode config
  • Implement OFF level for logger
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment