[WIP] SyliusResourceBundle Reloaded

[pjedrzejewski]

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	yes
Deprecations?	no
Fixed tickets	#1764 #2232 #2223
License	MIT
Doc PR	-

Do NOT review the code! It is a quick POC.

Hey folks!

These changes are partially inspired by another bundle (inspired by Sylius resource, lol), but I could not find it on github/packagist... Can be that it is gone, I have looked at it long time ago and it inspired me to work on this.

I wanted to do it for a long time and seems like it is a last moment to introduce a BC break of this size in Sylius fundamentals. It is an alternative approach to @stloyd's #1764 DomainManagers and effectively decouples Sylius components from the Doctrine Persistence interfaces, while staying very close to them, so that any Symfony developer will be used to the same API. Overview of my proposed changes:

• Introducting ResourceManagerInterface.

Every resource gets it's own dedicated resource manager. Additionally, it has this EvenfulResourceManager, which dispatches our beloved events.

Also, I have made it a bit smarter, and it has only save() and delete() methods. Save will know if it is a new or existing resource and dispatch an appropriate event. (update or create)

• Decouple the "createNew" from Repository interface and move it to dedicated ResourceFactoryInterface. The reason is that in Sylius we often want several ways to create a resource, for example create variant for product, create taxon for taxonomy and I found factories really useful when working on multi-stock inventory (top secret, yet). I believe dedicated factory is the right place, as opposed to putting it on the "DomainManager" in original PR from Joseph. (great work, just saying)
• Introduce ResourceEventDispatcherInterface. We have our resource events and I was tired of passing the strings around, to dispatch appropriate event name, now every resource gets a dedicated event dispatcher which can be used more easily.

Do not look at the resource controller yet, it needs some work and better flashes handling and so on. I will move them to a listener and generally, the controller will have all the previous functionality.

IMPORTANT NOTE
This change requires us to move to PHP 5.4 (ResourceInterface::getId). With recent decisions about PHP version for Symfony 3.0, I'm more than convinced to move to 5.4, especially, that we could do a good use of traits in many places. (in few we could hide design flaws too... by making our anemic models smaller in terms of LOC... joke)

Let me know what do you think.

[arnolanglade]

I like that :)

[makasim]

I followed your Do NOT review the code! and did not look at the code but the description looks really promising.

Finally 5.4. I like it.

[arnolanglade]

I just wrote some comments, I like it ! definitely 👍 for PHP 5.4!

[jjanvier]

It looks really interesting 👍 . Maybe the bundle you were looking for is this one https://github.com/ProgrammingAreHard/ResourceBundle ! It seems quite close in the spirit.

[arnolanglade]

@jjanvier yes you're true I found the original tweet : https://twitter.com/dadamssg/status/487048677127491584

[pjedrzejewski]

Yes, that's the one I was talking about. :)

[arnolanglade]

@pjedrzejewski we need that #1927 too, it is a bit boring to declare each time the resource forms. As I said there (#1921) we should rework the configuration too.

[peteward]

Looks really good on the whole!
Plus the PHP 5.4 bump will help everyone I'm sure.

[gonzalovilaseca]

@pjedrzejewski Correct me if I'm wrong, but with this changes, every 'save' performs a flush, right?
So the doctrine optimization of updating multiple tables in just one db connection is lost?

[pjedrzejewski]

@gonzalovilaseca Currently yes, but I plan to introduce optional second parameter to avoid that.

[arnolanglade]

@pjedrzejewski So it seems that people like it! What do you plan for that PR?

[pjedrzejewski]

Will try to merge few big thing before doing this, release v0.13.0 and finish this. :)

[stloyd]

In overall looks really similar to my changes, just a bit more decopuled.

From what I see there are few missing things, or parts I'm not convinced:

• multiple persist/delete without flush,
• why forcing to use ResourceInterface::getId() & PHP 5.4 (you could use ResourceInterface::getIdentifier() etc. and the issue would not popup probably)? Also that issue was fixed in PHP 5.3.9+ (I know that bump gives more features, but I'm not convinced to do this in this PR),
• no custom manager/factory to show how it's better than current code (i.e. cart or taxonomy hardcoded model), I know it's PoC but still you should add at least one IMO,
• I quite don't like calling call_user_func_array() instead of old approach, which IMO was more readable than this,
• I'm not sure about hardcoding class names in driver factories

That's all for now I think.

[michalmarcinkowski]

Great! 👍

[umpirsky]

Old scenario was better. :)

[peteward]

I can't comment inline because this PR is too big! but referring to this:

    /**
     * @param QueryBuilder $queryBuilder
     *
     * @return Pagerfanta
     */
    protected function getPaginator(QueryBuilder $queryBuilder)
    {
        return new Pagerfanta(new DoctrineORMAdapter($queryBuilder, true, false));
    }

It's good that you fixed the bug on the third parameter being null when it should be false :)

However the second parameter also has significant performance implications although it is very contextual.

In most cases, you do want this to be true, but when running a query on any large tables without joins or one-to-one associations (such as Customer JOIN User) then you really do not want to be adding a DISTINCT to the query - which the ORM Paginator does.

To illustrate the point, here's the query times taken to display the main Customers backend page for just under 1,000,000 customers on an i7 macbook pro:

$fetchCollectionJoin = true;

$fetchCollectionJoin = false;

I'm not particularly happy about 2 seconds for this simple query but 12 seconds isn't going to work. This is all down to the DISTINCT on the long query which is not necessary due to the nature of this query.

I think you need to allow the setting of this second parameter and I guess the only way is to support it is by passing into this method:

    /**
     * @param QueryBuilder $queryBuilder
     *
     * @return Pagerfanta
     */
    protected function getPaginator(QueryBuilder $queryBuilder, $fetchCollectionJoin = true)
    {
        return new Pagerfanta(new DoctrineORMAdapter($queryBuilder, $fetchCollectionJoin, false));
    }

It's a bit ugly for the other Adapter types to keep a consistent interface - having said that, most people are going to use MySQL here and this is a big roadblock for anyone with a big database.

How we can then allow queries to take advantage of this is another question, which can probably be answered after merge.

[pjedrzejewski]

@peteward Yes, I think I will reflect that in the interface... It is not perfect, but needed. Generally, we need to look for solutions with big amounts of data in backend. When Grids are merged, I was thinking we could add elastic search driver for them and use it for some of resources. It should be pretty simple to implement actually.

[peteward]

Great, thanks.

Don't worry, there will be plenty more examples of big data coming your way, with a million orders to be migrated next month...

[pjedrzejewski]

@peteward Yes, I already started thinking about possible solution for the cart + order issue... They can't live in a single table with 1M+ orders. ;)

[peteward]

@pjedrzejewski We have a caching layer in redis for carts, so this shouldn't be a problem (hopefully!).
We can share this if it's successful (i'm sure we could hook it into the sylius_cache mechansim).

[peteward]

so.... how are you getting on with this @pjedrzejewski? Do you need any more community help?

[pjedrzejewski]

@peteward Being away does not help, but this is my priority right now and I will rebase this PR with master this weekend and see if anyone can help with getting this green. Specs are passing and I think there are 2-3 suites to fix.

[aramalipoor]

@pjedrzejewski You're doing a great job it's very appreciated. What is the status of this PR? Is there anything we can do? Almost everything depends on this.

[pjedrzejewski]

Closing this as all of this stuff has been implemented. Yay!