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

[WIP] setcontent #116

Merged
merged 23 commits into from Dec 14, 2018

Conversation

@xiaohutai
Copy link
Member

commented Nov 16, 2018

Some basic features setcontent calls working, based on Bolt\Storage\Query\Query from Bolt 3.

This is the first of the setcontent iteration (based on @rossriley's work for Bolt 3).
Some complex situations arise because a content-type is not contained in one table anymore.

So we have bolt_content with a has-many relationship to bolt_field.

A distinction is to be made when query-ing:

  • Fields from the base table bolt_content. This is very simple and remains similar to Bolt 3's way for fetching content.
  • Fields from the fields table bolt_field. A bit more complex, because we need to use JOINs.

Basic

  • ✔️ {contenttype}
  • ✔️ {contenttype}/first/{number}
  • ✔️ {contenttype}/last/{number}
  • {contenttype}/random/{number}
    • random is not going to work with Doctrine ORM queries, see answer on Stack Oveflow for info.
    • An alternative is to make a special Twig function/filter to handle random records.
  • ✔️ {contenttype}/{id}
  • ✔️ {contenttype}/{slug}

WHERE

  • ✔️ {key}: {value}
  • ✔️ {key}: {operator}{value} where operator is one of % < > <= >=
    • ⚠️ Currently the operations will not work for numbers/dates/etc. because of JSON-encoded values.
  • ✔️ author: {id} replaces ownerid from Bolt 3

WHERE advanced

  • {key}: {value1} || {value2} or {key}: {value1} && {value2}
    • Currently only works for columns in bolt_content
  • "{key1} ||| {key2}": "{value1} ||| {value2}"
    • Currently only works for columns in bolt_content
  • {key}: '>now' and other special datetime selectors (e.g. now, today, yesterday, last year, next thursday).
    • Currently do not work at all.

Special

  • ✔️ limit {number}
  • ✔️ orderby {field} or orderby -{field}
    • ⚠️ This works for fields that have only a single value (think of it as a column)
    • ✔️ Multiple sortorders do work (comma-separated)
  • ✔️ returnsingle
  • ✔️ printquery
    • ⚠️ Currently shows DQL. Let's see if we can show a runnable query instead.

Advanced

Pagination and Search are not implemented yet.

  • allowpaging
  • {contenttype}/search/{num} where filter: {term}
  • {ct1},{ct2},{ctN}/search/{num} where filter: {term}

Misc

  • Implementations for locales, taxonomies and relationships.
  • Proper integration with Config + ContentType data (such as max results per page).
  • Remove all legacy stuff for real.
@bobdenotter

This comment has been minimized.

Copy link
Member

commented Nov 16, 2018

See #41

src/Repository/ContentRepository.php Outdated Show resolved Hide resolved
src/Storage/Query/Adapter/PostgresSearch.php Outdated Show resolved Hide resolved
src/Storage/Query/Adapter/PostgresSearch.php Outdated Show resolved Hide resolved
src/Storage/Query/SearchConfig.php Outdated Show resolved Hide resolved
src/Storage/Query/Adapter/PostgresSearch.php Outdated Show resolved Hide resolved
src/Storage/Query/Directive/LimitDirective.php Outdated Show resolved Hide resolved
src/Storage/Query/Directive/OffsetDirective.php Outdated Show resolved Hide resolved
src/Storage/Query/FrontendQueryScope.php Outdated Show resolved Hide resolved
src/Storage/Query/FrontendQueryScope.php Outdated Show resolved Hide resolved
src/Storage/Query/Handler/RandomQueryHandler.php Outdated Show resolved Hide resolved

@xiaohutai xiaohutai force-pushed the xiaohutai:feature/setcontent branch 3 times, most recently from ca8c331 to eacfc58 Nov 30, 2018

@JarJak

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

Should I put some feature requests here or in the related Epic?

@JarJak

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

random is not going to work with Doctrine ORM queries, see answer on Stack Oveflow for info.

Why do we use DQL for selects? This can be done with:

  • creating Read Model (flatten content with fields)
  • use simple SQLs without tons of JOINs
  • update Read Model on Entity flush

See:
https://ocramius.github.io/doctrine-best-practices/#/

Doing it this way would ease integration with API Platform.

Eventually this Read Model can, but not must, be stored in separate faster Storage, like Redis, ElasticSearch or even MongoDB.

Also, good to read:
symfony/symfony-docs#8893 (comment)

@bobdenotter

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

Should I put some feature requests here or in the related Epic?

This is primarily a branch by @xiaohutai to get a baseline, ported version of setcontent from v3 into v4.

Improvements and fixes should go into the accompanying epic and/or subsequent issues. :-)

@bobdenotter bobdenotter added this to the Bolt 4 alpha 1 milestone Dec 2, 2018

@xiaohutai

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2018

@JarJak I'll have to read up on that.

I've tried getting this up and running with the code that was available as a starting point (this may not have been the best idea). I think the most important part is that the results returned is consistent. So if a refactor/re-write is better, then we should definintely go for that.

(Any help is, of course, appreciated)

@JarJak

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

@xiaohutai the key thing here is that rewriting this feature to DQL and then back to SQL is a double work. DQL is super slow for complex queries (like listing things with joins) and should be avoided unless you don't really need an Entity object. You need Entity only when you are going to change and persist it. So, DQL is fine in admin area, but doing a couple of complex setcontent (DQL) calls inside a frontend twig template or api call can kill performance easily.

@xiaohutai xiaohutai force-pushed the xiaohutai:feature/setcontent branch from eacfc58 to 157a80d Dec 3, 2018

@JarJak

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

If we really need full entities in frontend, how about using Criteria instead of DQL?
https://www.doctrine-project.org/projects/doctrine-orm/en/2.6/reference/working-with-associations.html#filtering-collections

@xiaohutai xiaohutai force-pushed the xiaohutai:feature/setcontent branch 3 times, most recently from c816c89 to 5df2ef1 Dec 3, 2018

@xiaohutai

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2018

@JarJak Thanks for the resources. I'm don't have much Doctrine experience, so I need to read up on that. Both Doctrine ORM and the Query code from Bolt 3 is quite new for me (it's the non-legacy setcontent version).


Also: I've added a skip rule for AssignmentInConditionSniff.FoundInWhileCondition to easy-coding-standard.yml. Not sure how you feel about that. See: https://github.com/bolt/four/pull/116/files#diff-a8b950982764fcffe4b7b3acd261cf91

Review is outdated

@JarJak

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

@JarJak
Copy link
Member

left a comment

This PR can't be merged without at least functional tests.
Otherwise we will never be sure this works correctly.

src/Storage/Query/QueryParameterParser.php Outdated Show resolved Hide resolved
easy-coding-standard.yml Outdated Show resolved Hide resolved
src/Storage/Query/Directive/OrderDirective.php Outdated Show resolved Hide resolved
src/Storage/Query/Directive/OrderDirective.php Outdated Show resolved Hide resolved
src/Storage/Query/Directive/OrderDirective.php Outdated Show resolved Hide resolved
src/Storage/Query/QueryParameterParser.php Outdated Show resolved Hide resolved
src/Storage/Query/Handler/LatestQueryHandler.php Outdated Show resolved Hide resolved
src/Storage/Query/QueryParameterParser.php Outdated Show resolved Hide resolved
src/Storage/Query/QueryParameterParser.php Outdated Show resolved Hide resolved

xiaohutai added some commits Oct 30, 2018

Add `Bolt\Storage\Query` and `Bolt\Twig\TwigRecordsView` from Bolt 3
This is commit c3ead6d48cc2431d72ae7f81947df7fa5fefa92b in
https://github.com/bolt/bolt

Copied on 2018-10-30 14:00

Once Query works, it could be refactored from `Bolt\Storage\Query`
to `Bolt\Query`
(WIP) Refactor DBAL to ORM
Very rough early variant with some assumptions

@xiaohutai xiaohutai force-pushed the xiaohutai:feature/setcontent branch from 5df2ef1 to 5eba811 Dec 14, 2018

Run ECS check
I know that `$value` can be `string` or `integer`
@xiaohutai

This comment has been minimized.

Copy link
Member Author

commented Dec 14, 2018

To be honest I think that I am not not qualified for this. Doctrine ORM and the Query layer are both rather new to me. With the rest of the changes open and still features not working, I am not sure how to solve those properly.

Hopefully someone else with more experience can work on it and/or refactor it. I believe that's why Bob asked me to make a start with it. @bobdenotter, you'll have the final say.

Thank you @JarJak for all the feedback. Appreciated!

@JarJak

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

@xiaohutai You did a lot of good stuff here! @bobdenotter I think we (in TSH) can take care of this feature. It should be easier to implement using Criteria / Query Functions and a separate Read Model.

@bobdenotter

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

As discussed, merging in right now! This issue is quite large heavy already, so let's split off the lingering issues into separate issues, to keep them smaller and more manageable.

Thanks for the work @xiaohutai, and thanks @JarJak for the review and the offer to help out further! 👍 👍

@bobdenotter bobdenotter merged commit 72fda6e into bolt:master Dec 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@JarJak

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

@bobdenotter IMO this wasn't ready for merge... at least don't remove this branch please.
We should not merge features without tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.