DB gateway #81
DB gateway #81
Conversation
|
Move 0475d7e to separat PR? |
|
@alongosz : All review comments should be fixed now |
bundle/Command/Gateway.php
Outdated
| /** | ||
| * @license For full copyright and license information view LICENSE file distributed with this source code. | ||
| */ | ||
| namespace EzSystems\EzPlatformXmlTextFieldTypeBundle\Command; |
There was a problem hiding this comment.
Ideally this should be in "lib" and not bundle
So code in lib is meant to be tested, does not rely on symfony framework (but components are fine), while code in bundle are thin commands and controllers for symfony that typically don't5 have automated tests.
There was a problem hiding this comment.
Ideally this should be in "lib" and not bundle
Hmm, I didn't mind it "inside" Command namespace because of scope, but I'm also fine with changing that. However in that case we need better name than just Gateway.
Maybe ContentModelGateway? (because it covers both Content and ContentType aspects).
while code in bundle are thin commands and controllers for symfony that typically don't have automated tests.
btw. true, but that's bad, we've seen this approach failed us ;) // out of scope comment
There was a problem hiding this comment.
@andrerom : so move it to lib/FieldType/XmlText/ContentModelGateway.php ?
There was a problem hiding this comment.
so move it to lib/FieldType/XmlText/ContentModelGateway.php ?
Hmm, rather not directly in lib/FieldType/XmlText/.
I've just looked at existing files structure in this bundle... :)
Based on the convention we have in the Kernel, either:
lib/FieldType/XmlText/Persistence/Legacy/ContentModelGateway.php
orlib/FieldType/XmlText/Persistence/Legacy/Content/Gateway/DoctrineDatabase.php
but I think it's overkill here, so I'd suggest the former one.
There was a problem hiding this comment.
I moved it to lib/FieldType/XmlText/Persistence/Legacy/ContentModelGateway.php
There was a problem hiding this comment.
Forgot to move tests accordingly.
Fixed : /tests/lib/FieldType/Persistence/Legacy/ContentModelGatewayTest.php
|
@alongosz : review ping |
|
Better late than never: looks good now :) |
A fix for https://github.com/ezsystems/ezplatform-xmltext-fieldtype/issues/79