Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #39 - Adds MySQL TIMESTAMP and PgSQL DATETIME formats. #40

Merged
merged 2 commits into from
Dec 17, 2021

Conversation

brandonsavage
Copy link
Contributor

I'm not sure I put these in the right place, but if there's a better place, let me know.

I see this package being incredibly useful for automatically mapping data from databases into entities/value objects, so I hope that you'll consider this patch.

The performance is based on how soon we find the right format,
so we want to prefer more common formats (like database formats)
over less-common formats.

Also refactoring the tests to use more consistent language around
database types.
@romm romm merged commit 179ba3d into CuyZ:master Dec 17, 2021
@romm
Copy link
Member

romm commented Dec 17, 2021

Thank you very much for the contribution!

I see this package being incredibly useful for automatically mapping data from databases into entities/value objects, so I hope that you'll consider this patch.

You don't rely on an ORM and do the hydration of your value objects yourself? I'm just a bit curious about how you use the library. Happy it helps though. 🙂

@brandonsavage
Copy link
Contributor Author

Typically I build my own domain model rather than using an ORM. The logic is pretty straightforward and very easy to test. In this case, I would be populating value and entity objects from raw data pulled from the database.

Sample code:

    public function findUserByEmail(string $email) : ?User
    {
        $select = $this->queryFactory->newSelect();
        $select->cols(['*'])->from('user_account')->where('email = :email');
        $result = $this->pdo->fetchOne($select, ['email' => $email]);
        if (!$result) {
            return null;
        }

        if ($result['last_login']) {
            $result['last_login'] = [
                'datetime' => $result['last_login'],
                'format' => 'Y-m-d H:i:s.u',
            ];
        }

        return (new MapperBuilder())->mapper()->map(
            User::class,
            $result
        );

    }

@romm
Copy link
Member

romm commented Dec 18, 2021

I see; I am also trying out DDD with a customer, but we still use Doctrine's ORM although we already thought about not using it at all (at least for read-models); we'll maybe reconsider this option.

Thanks again for the contribution. 🙂

@Mediagone
Copy link

Mediagone commented Dec 18, 2021

Typically I build my own domain model rather than using an ORM.

Domain model has nothing to do with the usage of an ORM or not.

The Domain Model is the ensemble of all entities that fit your business requirements. Then, you have several ways to bind it with your persistence layer (database): you can use an ORM to do the work automatically or hydrate objects by yourself.

Notice that you still use a kind of ORM (in you example code), by requiring a "mapper" to build your object (an ORM being somehow a "mapper"). So what's the point to replace Doctrine ORM with this custom MapperBuilder?
A simpler solution for manual hydration would be: return User::fromArray($result);

If the point to drop Doctrine ORM was performance, you should know that it's generally caused by a poor usage of the ORM rather than the ORM itself. You can, for example, use the NEW DQL operator to hydrate DTOs instead of entities.
( see: https://www.doctrine-project.org/projects/doctrine-orm/en/2.9/reference/dql-doctrine-query-language.html#new-operator-syntax )

Some useful links:
https://www.slideshare.net/guilhermeblanco/orm-dont-kill-your-db-developers-do
https://ocramius.github.io/doctrine-best-practices/#/

Comment on lines +32 to +33
$mysqlDate = '2012-12-21 13:37:42';
$pgsqlDate = '2012-12-21 13:37:42.123456';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm honestly wondering whether this test with these data is any useful. The current mapper's implementation is brute-forcing through various formats, using the first one that works.
The mechanism itself may be fine, though in this test each and every use-case is testing against the very same date time, potentially allowing/hiding false-positives where one format succeeds over the one that was supposed to be tested.

My proposal here would be to change each test-case to a different datetime to ensure that each test is actually doing what it's supposed to do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good advice, thank you for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants