Comment created by eugene-d:
doctrinebot on 28 Jul 2015
This is actually really confusing definitely when you combine it with the docs
boekkooi on 8 Jan 2016
π1
I must say I find this rather a bug than an improvement... agree with @boekkooi regarding the docs...
afoeder on 11 Feb 2016
Can anyone confirm if this is a dupe of #4670 and #4568?
Ocramius on 11 Feb 2016
well this _is_ #4568; and #4670 is rather a duplicate of #1275, both actually "features" or "improvements".
This here however is a Bug because embeddables whose all properties are nullable and being null hydrate to an empty object when being stored as null. This is misleading (saving != retrieving) and not according to the docs.
afoeder on 11 Feb 2016
@afoeder the order of the tickets is scrambled due to the fact that they were imported from Jira in December, heh
Ocramius on 11 Feb 2016
Hi @Ocramius
Has there been any further discussion on this topic? We've just hit into the same problem as described here on our first Doctrine project.
Let me know if there's anything we can do to help β provide usage examples, code samples, discussions, etc.
Harrisonbro on 5 Sep 2016
@Harrisonbro as it currently stands, doctrine will not support nullable embeddables. That functionality may be implemented later, by implementing embeddables as hidden one-to-one records.
Ocramius on 7 Sep 2016
OK. Is there any way for us to implement this on a case-by-case basis (eg. by hooking into the hydration process of an embeddable somehow) so we can manually check whether specific embeddables have enough data in the database to be considered 'valid' and therefore hydratable? Obviously we're not keen to allow value objects to be instantiated in an invalid state and then check an isValid
method.
Harrisonbro on 8 Sep 2016
so we can manually check whether specific embeddables have enough data in the database to be considered 'valid' and therefore hydratable?
Then just use the lifecycle system to (de-)hydrate VOs on your own, no?
Ocramius on 8 Sep 2016
Then just use the lifecycle system to (de-)hydrate VOs on your own,
no?
Do you mean lifecycle callbacks β maybe postLoad
β as shown in
http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/events.html#lifecycle-callbacks?
Looks like those only work on entities, not value objects, as far as I
can see? Eg. if I used postLoad
an embeddable will already have been
hydrated with invalid data (if the data in the database is all null
,
for example). Alternatively, if I move the VO properties onto the entity
directly Iβve lost the nice encapsulation that embeddable so usefully
provides (eg. if I had a Product
entity with a SalePrice
with 2
properties, value
and currency
Iβd have to move those 2 properties
onto the entity. Whilst I could then have those properties be private
and do an is_null
check for those 2 properties before instantiating
and returning the VO from getSalePrice() : SalePrice { β¦ }
it does
rather compromise my entity.
Iβm almost certainly missing something here, sorry. Rather new to
Doctrine so still learning!
Harrisonbro on 8 Sep 2016
@Harrisonbro the idea is to NOT use embeddables there, and use a lifecycle listener to replace fields with embeddables then (manually). Doctrine will not implement nullability for embeddables for now.
Ocramius on 8 Sep 2016
@Harrisonbro the idea is to NOT use embeddables there, and use a lifecycle listener to replace fields with embeddables then (manually). Doctrine will not implement nullability for embeddables for now.
OK, gotcha.
So in the example I gave β a Product
entity which wants to use a SalePrice
VO with 2 fields, amount
and currency
β would you suggest simply putting a sale_price_amount
and sale_price_currency
property on the Product
entity, make those private, and then have Product::getSalePrice() : SalePrice
first check whether the 2 properties are null
before attempting to instantiate and return the VO?
If so, that seems workable and means the entity is responsible for checking if the VO should be instantiated (rather than having the VO able to be βinvalidβ and have to implement an isValid()
method).
Example code of what I mean:
class Product { private $sale_price_amount; private $sale_price_currency; public getSalePrice() : SalePrice { if ( is_null($this->sale_price_currency) || is_null($this->sale_price_amount) ) { return null; } return new SalePrice( $this->sale_price_currency, $this->sale_price_amount ); } }
Is that something like what youβre suggesting instead of nullable embeddables?
Harrisonbro on 8 Sep 2016
So in the example I gave β a
Product
entity which wants to use aSalePrice
VO with 2 fields,amount
andcurrency
β would you suggest simply putting asale_price_amount
andsale_price_currency
property on theProduct
entity, make those private, and then haveProduct::getSalePrice() : SalePrice
first check whether the 2 properties arenull
before attempting to instantiate and return the VO?
Correct.
Basically, since this is a scenario that Doctrine can't cover right now (because of how RDBMS DDL works), you can just implement it in userland for the few times where it pops up.
Ocramius on 8 Sep 2016
OK great, thanks a lot for the help.
Harrisonbro on 8 Sep 2016
Thanks, I silently kept up reading your conversation :)
At the moment, my workaround is the following:
class Site{ /** * @var DomainName * @ORM\Embedded(class="DomainName", columnPrefix="domain_") */ private $domainName; public function domainName() { return ((string)$this->domainName === '' ? null : $this->domainName); }}/** * @ORM\Embeddable */class DomainName{ /** * Note this is only nullable in order to get the whole embeddable nullable (see [1] and [2] * * @var string * @ORM\Column(nullable=true) * @see http://doctrine-orm.readthedocs.org/projects/doctrine-orm/en/latest/tutorials/embeddables.html#initializing-embeddables [1] * @see https://github.com/doctrine/doctrine2/pull/1275 [2] */ private $name; /** * Note this is only nullable in order to get the whole embeddable nullable (see [1] and [2] * * @var string * @ORM\Column(name="escaped_name", nullable=true) * @see http://doctrine-orm.readthedocs.org/projects/doctrine-orm/en/latest/tutorials/embeddables.html#initializing-embeddables [1] * @see https://github.com/doctrine/doctrine2/pull/1275 [2] */ private $escapedName; public function __construct($name) { Assertion::notEmpty($name, 'The domain name must be provided.'); Assertion::regex($name, '/^(?!www\.)([\pL\pN\pS-]+\.)+[\pL]+$/u', 'The domain name "%s" must be a valid domain name without the www. subdomain, but might have others.'); $this->name = $name; $this->escapedName = static::escapeDomainName($name); } public function containsSubdomain() { return substr_count($this->name, '.') >= 2; } public static function escapeDomainName($name) { return preg_replace('/\./', '-', $name); } public function __toString() { return (string)$this->name; }}
afoeder on 8 Sep 2016
I like that approach, @afoeder β you still do an is_null
check in the entity's getter method (Site::domainName()
in your case) but you can still use an embeddable rather than having to hydrate your VOs yourself.
I suppose the major downside of your approach is that you do still have a VO in an inconsistent state, whereas if you don't let Doctrine hydrate the VO as an embeddable you avoid this; a bit more boilerplate & checking code, but you never have a VO in an invalid state.
Really it's just a trade-off between the 2 options. Others reading this should just be aware of the 2 options and their various merits.
Harrisonbro on 8 Sep 2016
I would have preferred to comment on #1275, but the present issue has the benefit to be still open.
My 2 cents on the sensitive subject of nullable embedded properties:
- when the Embeddable has at least one non-nullable
@Column
, and this field is null in the database, there should be no ambiguity andnull
should be assigned to the embedded property. Otherwise (currently!) you get an empty, invalid value object that has non-nullable properties set tonull
. IMHO, the current implementation is broken here. - when all
@Column
in the Embeddable are nullable, there should be a boolean setting in the@Embedded
annotation that controls whether or not you want an empty value object or anull
value when all fields arenull
in the database. Your choice.
Finally, you only have a real problem when you have a fully nullable embeddable, and want to make the distinction between a null
property and an empty object. People have suggested to add an extra column in the table, which would work, but would add a ton of complexity for what I think is an edge case. To clarify, I think this edge case should not be supported by Doctrine.
The previous two bullet points, however, I would strongly suggest working on them ASAP. I'll be happy to help, provided that lead developers are happy with the concept.
BenMorel on 3 Feb 2017
π7π3
Thanks for that clear explanation, @benmorel. I quite agree with your suggested specification of how Doctrine _should_ behave, and that it should be worked on. This issue is the top priority I'd like to see addressed in doctrine.
I too would be happy to help out with the development and testing of this, if the Doctrine team agree.
Harrisonbro on 3 Feb 2017
π1
We came across the same issue, in our domain a StreetAddress
is optional, but if given, it has to contain all fields. All fields on the Embeddable
are nullable: true
, so the DB is working. The domain is ensuring valid state. So I created that listener to make Doctrine load the objects the way the domain contains objects prior persisting: https://gist.github.com/havvg/602055f1488271f68e5bc82f9a828b4d
Well, it only requires knowledge on the embeddable itself, but easy workaround for now. I hope this helps other developers until the issue will be resolved by Doctrine.
havvg on 26 Mar 2017
π1
@havvg How to use this workaround?
For example I have User entity with embeddable class Gender. Something like that:
https://gist.github.com/szepczynski/d3028eb9f92fd7aadd08a578c7a92ad3
I know that I need the User entity should have registered postLoad listener NullableEmbeddableListener but I have no idea how to register it. Can you provide any example? I guess that I need somewhere call addMapping?
szepczynski on 27 Apr 2017
@BenMorel did you by any chance work on a PR for this? I'd really hope to have this "bug" resolved, but I don't understand doctrine well enough to be able to write a pretty PR for this issue.
Evertt on 8 May 2017
@Evertt Not yet, and I won't until I get the green light from lead developers. I've invested a lot of time in other pull requests, that have been open for years and are still not merged. I can't waste any more time on this project I'm afraid!
BenMorel on 8 May 2017
@BenMorel that's too bad and I totally understand! It's sad that huge projects like these at some point seem to slow down to a point that it just seems frozen.
Evertt on 8 May 2017
That's the dark side of open source: projects rely solely on the free time developers can invest in them, and at some point they're just too busy on other businesses and/or family life to carry on with developments.
I, too, feel like Doctrine is slowing down; it's just unfortunate that there aren't enough (available) lead developers to keep up the pace with pull requests: many developers are there to offer their help, but without enough consideration from project leaders, it's just wasted brain processing time.
BenMorel on 8 May 2017
π1
@BenMorel the pace did slow down a bit last year, but the last months have been quite active :) Also see #6211
theofidry on 9 May 2017
doctrine 2.x is frozen because doctrine 3 is actively developing (I read somewhere post by @Ocramius)
szepczynski on 9 May 2017
@szepczynski except that there's no ETA for doctrine 3 so that could take years for all we know. If it really takes that long it would be nice for improvements to still be added to doctrine 2.
Evertt on 9 May 2017
If it really takes that long it would be nice for improvements to still be added to doctrine 2.
It would make it a mess to migrate these additions to something completely redesigned. From what I can see in the last dozen releases, doctrine functionality already abundantly covers the 90% of use-case scenarios, so we could even call it "feature complete", if it wasn't for some rough edges that you encounter when you explore more shady features.
Ocramius on 9 May 2017
@Ocramius I wouldn't call this issue right here a "shady feature". I think this is a very essential part of the embeddables system.
Evertt on 9 May 2017
π5
Right, and embeddables have barely been added in 2.5
, and are already removed in develop
(3.x
), as their fundamental internal working mechanisms need to be rewritten
Ocramius on 9 May 2017
@Ocramius Sorry to pollute this thread, but would the transaction object and default lock mode fit in 3.0?
BenMorel on 9 May 2017
@BenMorel most likely, yes
Ocramius on 9 May 2017
Right, and embeddables have barely been added in 2.5, and are already removed in develop (3.x), as their fundamental internal working mechanisms need to be rewritten
@Ocramius I'm not sure I understand you right. Do you mean they will come back in 3.x after their internal working mechanisms have been rewritten?
Evertt on 9 May 2017
@Evertt yes, but likely as completely rewritten/redesigned.
Ocramius on 9 May 2017
@Ocramius Are there ways we can help the development of Doctrine, either v2 or v3? It's a tool we all use so would love to,support development if a can.
_(Sorry to pollute this thread but not sure where else to write.)_
Harrisonbro on 10 May 2017
@Harrisonbro https://github.com/doctrine/doctrine2/milestones/3.0
Let's please stop going further OT. If you have a question, make a new issue.
Ocramius on 10 May 2017
FTR: there is a small 3rd party library that provides a listener for setting embedded entities that are all null to null (the gist that was discussed above): https://github.com/tarifhaus/doctrine-nullable-embeddable
dbu on 3 Aug 2017
@BenMorel With all due respect, I have to go r/quityourbullsh*t on you here:
I've invested a lot of time in other pull requests, that have been open for years and are still not merged. I can't waste any more time on this project I'm afraid!
For Doctrine 2, there are 26 pull requests from you: 2 Open, 2 Closed without merge (one was fixed differently, one would introduce a lot of pain with future pull requests), and 22 merged.
For DBAL, there are 15 pull requests from you: 1 open, 2 Closed without merge, and 12 merged. A quick peek into other repositories (common, annotations, bundle, etc.) shows merged pull requests only.
Feel free to point out pull requests that you are waiting to get merged, but please don't say stuff like that without backing it up when other people sacrifice lots of free time to get you free software. Thank you.
alcaeus on 4 Aug 2017
π1
@alcaeus Thanks for investing your time investigating my contributions to this repository.
You have already wonderfully pointed out my unmerged pull requests, you just forgot to mention their opening date:
- doctrine/dbal#634 : opened 3 years ago
- doctrine/doctrine2#949 : opened 3 years and 6 months ago
I did invest quite a lot of time on these two pull requests, and since 2 years I am getting next to no feedback, despite multiple attempts to draw attention from the team.
Yes, I had PRs merged as well (did I ever say I didn't?), but the lack of feedback on these last two blocked my motivation to contribute further to this project for now.
[...] please don't say stuff like that without backing it up when other people sacrifice lots of free time to get you free software.
I hope I have backed it up to your taste, and rest assured that I know quite well what it's like to sacrifice some time on free software.
Cheers.
BenMorel on 5 Aug 2017
π1
@BenMorel the way you made it sound was that people completely disregarded your work, which they don't. That's why I took the time to look through your contributions to see what's going on.
As for the pull request you mentioned, (please keep in mind that I'm not an ORM guy) it looks like it's a fairly large pull request that touches transaction logic in the DBAL, affecting most of what it (and thus ORM) does. Pull requests like that take time to review and evaluate. I'm not trying to make excuses for other people here, I'm just hoping you can bear with the people maintaining it. That said, there is currently a development push going for 3.0, so maybe there's an opportunity to get those pull requests merged.
Open source projects need contributors to move forward and evolve, especially when maintainers have little or in some cases no time for the project. However, writing pull requests is just a small part of that work - most of it is looking at issues, figuring out what's going on, evaluating pull requests and making sure your user base is not going to burn you at the stake because you messed up. That can be very time consuming and tiring, so unfortunately, large pull requests are often the first to stay open simply because of the effort it takes to review and merge them.
alcaeus on 5 Aug 2017
@BenMorel I agree with you. The Doctrine mantra I keep hearing seems to be along the line of "Well, it kind of works in most cases and changing things is hard so let's not do it".
They say a picture says a thousand words and I think this picture sums up my feelings about Doctrine right now:
Disclaimer: Owners, don't take offense - it's meant in a lighthearted way and is just my personal opinion. I know you work hard on this, and for that I thank you. I just disagree a bit with the general negativity that I personally see towards any major changes. I know you're working on 3.0 but Symfony and others are leaving you in the dust. If there's too much work, consider giving other contributors more rights or bumping the major version more often so that there can be BC breaks.
ryall on 25 Jan 2018
@ryall I think that's the nail on the coffin then.
Here's the exit:
Closing and locking.
Ocramius on 25 Jan 2018
Was this page helpful?
0 / 5 - 0 ratings