Doctrine DBAL
  1. Doctrine DBAL
  2. DBAL-628

Moved public properties out to getters/setters

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Trivial Trivial
    • Resolution: Won't Fix
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Security Level: All
    • Labels:
      None

      Description

      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

        Activity

        Hide
        Christophe Coevoet added a comment -

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

        Show
        Christophe Coevoet added a comment - Regarding the end of your description, Doctrine DBAL is not a Symfony project. It is a Doctrine project.
        Hide
        Christophe Coevoet added a comment -

        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

        Show
        Christophe Coevoet added a comment - 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
        Hide
        Danny Kopping added a comment -

        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?

        Show
        Danny Kopping added a comment - 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?
        Hide
        Christophe Coevoet added a comment -

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

        Show
        Christophe Coevoet added a comment - Ignoring some tables is already supported through the schema filter: https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Configuration.php#L92
        Hide
        Danny Kopping added a comment -

        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.

        Show
        Danny Kopping added a comment - 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.
        Hide
        Christophe Coevoet added a comment -

        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)

        Show
        Christophe Coevoet added a comment - 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)
        Hide
        Danny Kopping added a comment -

        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

        Show
        Danny Kopping added a comment - 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
        Hide
        Marco Pivetta added a comment -

        This won't be fixed until 3.x

        Show
        Marco Pivetta added a comment - This won't be fixed until 3.x

          People

          • Assignee:
            Marco Pivetta
            Reporter:
            Danny Kopping
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: