Skip to content

Loading…

DBAL-628: Moved public properties out to getters/setters #1844

Closed
doctrinebot opened this Issue · 9 comments

2 participants

@doctrinebot

Jira issue originally created by user dannykopping:

Having many publicly-accessible properties in most of DBAL's classes makes it difficult to extend the library, and this does not conform to other Symfony projects

@doctrinebot

Comment created by stof:

Regarding the end of your description, Doctrine DBAL is not a Symfony project. It is a Doctrine project.

@doctrinebot

Comment created by stof:

And I checked quickly the use of public properties in DBAL 2.4. I only found them in 4 classes: the DebugStack logger and E classes in the Schema tool (ColumnDiff, TableDiff and SchemaDiff), which are value objects (ColumnDiff and TableDiff don't contain any logic at all).
So I would not call it "most DBAL classes". None of the classes which may have a reason to be extended are not using public properties

@doctrinebot

Comment created by dannykopping:

Fair enough - I may have been a little hyperbolic in my description.
The problem is that those 4 classes are used throughout the library - and this makes things a little tricky. For example, I wanted to add some functionality where certain tables, columns or indexes could be ignored when generating the migration SQL, and because only public properties were used - one cannot simply override a getter to add an 'ignore' feature.

Any thoughts?

@doctrinebot

Comment created by stof:

Ignoring some tables is already supported through the schema filter: https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Configuration.php#L92

@doctrinebot

Comment created by dannykopping:

Oh awesome - thank you! So you don't see any merit to move all of these out to getters/setters? My particular issue may have been a case of simply overlooking a feature, but could this type of class design not affect things later on? I've already spent a few hours moving the codebase over to getters & setters; if there's no point, i'll quite happily throw away the code.

@doctrinebot

Comment created by stof:

For value objects, using public properties is fine IMO. They should not hold logic (if you are trying to extend them, you are probably extending the wrong part of DBAL)

@doctrinebot

Comment created by dannykopping:

OK, well - I've done the changes and all the tests pass, so I'll push my code up and if at any point these changes are needed - they're be in a PR. Thanks

@doctrinebot

Comment created by @ocramius:

This won't be fixed until 3.x

@doctrinebot

Issue was closed with resolution "Won't Fix"

@doctrinebot doctrinebot added the Bug label
@Ocramius Ocramius was assigned by doctrinebot
@doctrinebot doctrinebot closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.