Promote private properties and methods to protected in `QueryBuilder`

[sbuerk]

Q	A
Type	improvement
Fixed issues	n/a

Summary

QueryBuilder may be used in a decorator pattern, to enhance the provided functionality. Private properties and methods makes it nearly impossible to do the same job without copying all of the private properties and methods and increasing the ongoing needed maintenance burden.

This change promotes the private properties and method up to protected accessibilty, which provides the same surface from the outside but making frameworks and application lives easier to provide enhancements with a minimal maintanence burden.

[derrabus]

QueryBuilder may be used in a decorator pattern

The decorator pattern is precisely the pattern that you don't need protected properties for. Please explain why you need this change. We won't accept a PR that simply switches everything to protected without a good reason or tests that cover the extension points that are introduced by that change.

[sbuerk]

QueryBuilder may be used in a decorator pattern

The decorator pattern is precisely the pattern that you don't need protected properties for. Please explain why you need this change. We won't accept a PR that simply switches everything to protected without a good reason or tests that cover the extension points that are introduced by that change.

Thanks for the answer. I will add a detailed explanation and tests to the PR on the weekend.

[sbuerk]

@derrabus Sorry for the longer delay, that was not intented.

Removed the QueryBuilder changed for now and added a test which contains a custom QueryBuilder class with most of the things we need and the internal wrapped QueryBuilder instance.

The main point of the wrapping is, to provide automatic identifier and in most cases value quotings out of the box. However, a direct access to the wrapped QueryBuilder is hold so direct plain and unquoted calls to all the methods are possible.

A further requirement we have from a application/framework point of view is, that different automatic constraints (joins, where etc) needs to be applied directly and for the joined conditions. These should only applied directly before the query is executed / build and removed after wards.

To make this even more wilder, even this wrapped QueryBuilder with the quoting stuff in place is considered lowlevel api and the application/framework provides additional stuff upon it, for example a ORM system (not doctrine/orm) and a custom configuration language which can be used to build some queries with quite some logic like placeholder replacements.

For these implementation access to the type of the QueryBuilder type set by used methods and all the query parts which have been consideres as internal implementation details and therefore removed the getQueryPart(), getQueryParts() methods and access to the QueryType.

Don't get me wrong, at one point I get it why it has been considered internal - and for most normal usages this may be okay, but a heavy system using this in a more enhanced way to provide the own developer community are more easier access to the Doctrine DBAL API with additional structures build on it, this renders this now to a maintenance nightmare.

To mitigate the private properties with no protected or public getters the while class have to be extended, absolutly each property copied with protected visibility along with all methods public or private and avoiding to call the parent constructor,

use Doctrine\DBAL\Query\QueryBuilder as DoctrineQueryBuilder;

class ConcreateQueryBuilder extends DoctrineQueryBuilder
{
   protected array $select = [];
   protected CompositeExpression|string|null $where = null;
   // ... all other properties, making private one protected
   
   public function __construct(protected Connection $connection) {}

    /**
     * copy & pasted from Doctrine QueryBuilder.    
     *
     * Specifies an item that is to be returned in the query result.
     * Replaces any previously specified selections, if any.
     *
     * <code>
     *     $qb = $conn->createQueryBuilder()
     *         ->select('u.id', 'p.id')
     *         ->from('users', 'u')
     *         ->leftJoin('u', 'phonenumbers', 'p', 'u.id = p.user_id');
     * </code>
     *
     * @param string ...$expressions The selection expressions.
     *
     * @return $this This QueryBuilder instance.
     */
    public function select(string ...$expressions): self
    {
        $this->type = QueryType::SELECT;

        $this->select = $expressions;

        $this->sql = null;

        return $this;
    }
    
    // ... copy & pasted all other private, protected and public methods due to
    // visibilty access constraints
}

This allows us then, to extend our enhanced QueryBuilder from this extended class and pass that class itself as internal QueryBuilder (unlike the unchanged doctrine one as before)

class ApplicationQueryBuilder extends ConcreateQueryBuilder
{
  public function __construct(
    protected ConreteQueryBuilder $concreteQueryBuilder,
    protected Connection $connection,
  ) {
    parent::__construct($connection);
  }
  
  public function from(...): self;
  {
    $this->concreteQueryBuilder->from(...);
    return $this;
  }
  
  public function getFrom(): array
  {
    // or ->getFrom() if proptect method added
    return $this->concreteQueryBuilder->from;
  }  
}

This basically works, but means that we have to check and update the copied methods on every patch level and so on and pull in new methods to the ConcreteQueryBuilder, like for example the added reset* methods (as replacement for the resetQueryParts() and resetQueryPart() methdos). If not not and called, they wont work if they operate on the private propeties of the parent class.

Maybe I'm fully wrong and you can me advise a simple way to mitigate this without the ConreteQueryBuilder in between. The only way which I see and have been tests by directly changing the doctrine querybuilder was to change the properies from private to protectec and the same for private methods.

I guess, at least having protected (internal) methods for the queryparts and leaving the properties private may work also to directly access the values through the getters from the wrapping QueryBuilder. On the other hand, having the properites made protected would avoid to clone the intermediate class ConcreteQueryBuilder and keep the doctrine one for this.

For other things we have gone the way to extend and do that, like for the SchemaDiff, TableDiff and ColumnDiff classes (where we hopefully can change the usage with a refactoring to drop that at some point again).

But for the QueryBuilder and the required access to the query part information is essential and I don't see a way to mititage this throughout the whole chain.

As an additionall example, we also have a extendes ExpressionBuilder which needs access to the join information to check for already set joins and aliases to avoid double registrations etc.

Granted, I had a lot to do to prepare the upgrade to Doctrine DBAL 4 - but the QueryBuilder part is the only part which is really annoying (not to be offending). At least thinking about the ongoing maintancens regarding doctrine dbal releases and internal changes which can be done easily for private properties and methods.

The original PR to simply change this was a quick side step done during a train travel, and to early without further explanation.

I hope, I could at least shed some light on our requirements and nightmares with the test and the explanation.

I'm open for any suggestion how to mitiage this without opening it completly public again (the query parts).

I'm also available in the doctrine slack as stefan.​buerk for a discussion (dm, huddle etc).

[github-actions[bot]]

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. If you want to continue working on it, please leave a comment.