Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GridBundle] [WIP] PHPCR-ODM Driver #4813

Merged
merged 1 commit into from
May 12, 2016

Conversation

dantleech
Copy link
Contributor

Q A
Bug fix? no
BC breaks? no
Deprecations? no
Related tickets
License MIT

This PR would introduce a PHPCR-ODM driver.

The main problem is building the query builder - the PHPCR-ODM query builder uses a different design than that of the standard Doctrine Query Builder, which makes building the query more difficult.

In order to solve this this PR uses the Doctrine Commons expresion builder and implements a visitor for that to build the PHPCR-ODM query.

If this approach seems reasonable I will finish this off and add the tests (PHPSpec or PHPUnit?)

*/
public function like($field, $pattern)
{
return $this->expressionBuilder->contains($field, $pattern);
Copy link
Contributor Author

@dantleech dantleech Apr 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"like" doesn't exist in the Collections QB API, bu we can actually return a custom comparison as new Comparison($field, 'like', $pattern), same with notLike.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like is analasgous to contains if I understand correctly (it is referenced to as "contains" in the filter).

public function isNotNull($field)
{
throw new \BadMethodCallException('Not currently supported.');
return $this->expressionBuilder->isNull($field);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should be removed if we throw exception above.

@pjedrzejewski
Copy link
Member

Hey @dantleech, thanks for this contribution!

If it is really not possible to use PHPCR query builder directly, then this sounds like a smart workaround. I'd say go for it. It does not make sense to add specs for existing code, so spec only new stuff or code that does not work as expected. :)

@dantleech
Copy link
Contributor Author

Updated, we can now build the query for PHPCR-ODM fom the Doctrine Collection Expressions.

I added the test for this with PHPUnit (it is more of a light-weight functional test) and created a simple helper class to generate a string representation of the query builder state (to make testing easier).

Next steps would be to add specs for the other classes and finish this off.

@pjedrzejewski pjedrzejewski changed the title [GirdBundle] [WIP] PHPCR-ODM Driver [GridBundle] [WIP] PHPCR-ODM Driver Apr 20, 2016
@pjedrzejewski
Copy link
Member

Hey @dantleech, we are going to need this real soon, so I will be happy to help with finishing this. Do you have some time? :)

@dantleech
Copy link
Contributor Author

hey @pjedrzejewski , yeah I can work on it tomorrow. I was waiting on #4812

@dantleech
Copy link
Contributor Author

Updated, all of the expressions are supported now and manually testing it seems to work fine for the String and Boolean filters. Still some work to do however.

*/
public function comparison($field, $operator, $value)
{
throw new \BadMethodCallException('Not supported yet.');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pjedrzejewski when does this get used?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nowhere yet, but I wanted to have it on the interface. Let's keep it for now like for other drivers.

public function isNull($field)
{
// could probably do this by comparing with eq = NULL and handling this case
// in the visitor.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove comment

@dantleech dantleech force-pushed the phpcrodm_driver branch 2 times, most recently from 817312e to 806fbda Compare May 11, 2016 10:06
/**
* @param QueryBuilder $queryBuilder
*/
function __construct(QueryBuilder $queryBuilder, ExpressionBuilder $expressionBuilder = null)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public

</parameters>

<services>
<service id="sylius.grid_driver.doctrine.orm" class="%sylius.grid_driver.doctrine.phpcrodm.class%">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Service id is invalid.

@dantleech dantleech force-pushed the phpcrodm_driver branch 2 times, most recently from 1df1aba to 7be50d9 Compare May 12, 2016 09:36
return $node;
}

public function dispatch(Expression $expr, $parentNode = null)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doc

@dantleech dantleech force-pushed the phpcrodm_driver branch 2 times, most recently from 9d3e8c1 to 37c1eee Compare May 12, 2016 09:46
@dantleech
Copy link
Contributor Author

dantleech commented May 12, 2016

Updated -- implemented order by - so that should be the final nail in this PR from a feature POV.

One thing I didn't find out how to test was ordering the grid from the UI - I notice that in the docs there is a sortable field in the (column) field definition, which seems to be missing from the bundle currently.

@pjedrzejewski pjedrzejewski merged commit cc9983c into Sylius:master May 12, 2016
@pjedrzejewski
Copy link
Member

Awesome work, this is a huge help for us, as we will soon add PHPCR stuff to new admin. Thank you Dan! 👍 We will absolutely iterate on this in separate PRs.

Regarding your question, sorting is not yet that functional, I will be looking at it soon for the UI and ORM driver.

I have one question though: why we need to apply sorting in the DataSource and not inside of the ExpressionBuilder?

@dantleech
Copy link
Contributor Author

Thanks :)

why we need to apply sorting in the DataSource

The expression builder is just a factory for Expr classes. It returns either Comparison or Composite (or similar) classes - unlike the Doctrine ORM ExpressionBuilder which acts directly upon the ORM QueryBuilder. The PHPCR-ODM query builder has no access to the actual query builder.

I just store it in the ExpressionBuilder itself, and we can then later apply it to the actual PHPCR-ODM query builder in the DataSource.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants