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.

Sign up for GitHub

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

Jump to bottom

[Data Loader] Add a chain loader implementation #953

Merged
merged 1 commit into from
Jun 28, 2017
Merged

[Data Loader] Add a chain loader implementation #953

merged 1 commit into from
Jun 28, 2017

Conversation

robfrawley
Copy link
Collaborator

@robfrawley robfrawley commented Jun 20, 2017 •
edited
Loading

Q A
Branch? 1.0
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #948
License MIT
Doc PR

Chain data loader implementation. This allows one to define one "filter set" that searches multiple loader configurations (for example, this allows you to check both a Gaufrette stream loader and a file system loader simultaneously).

Here is an example configuration for the loader:

liip_imagine:

    loaders:

        foo:
            filesystem:
                # setup this loader here

        bar:
            flysystem:
                # setup this loader here

        baz:
            chain:
                # name the loaders to chain
                loaders: [foo, bar]

    filter_sets:

        my_custom_filter_set:
            data_loader: baz
            filters:
                # define your filters here

This implementation simply loops through the available loaders and selects the first valid result, so if the file exists in multiple loaders, the first defined in your configuration will be selected. For example, if file.ext exists in both the foo and bar loader, the one found via the foo loader will be returned because it is defined first for baz.chain.loaders.

@robfrawley robfrawley mentioned this pull request Jun 20, 2017
Closed
alexwilson
alexwilson previously requested changes Jun 20, 2017
View reviewed changes
Tests/Functional/Binary/Loader/ChainLoaderTest.php Outdated

$loader = $this->getLoader('baz');

foreach (['images/cats.jpeg', 'images/cats2.jpeg', 'file.ext', 'bar-bundle-file.ext', 'foo-bundle-file.ext'] as $file) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shorthand array syntax breaking PHP 5.3 builds

Binary/Loader/ChainLoader.php Outdated
*/
private function getLoaderNamesString()
{
$names = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shorthand array syntax is breaking PHP 5.3 builds.

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://travis-ci.org/liip/LiipImagineBundle/jobs/244807011

Tests/DependencyInjection/Factory/Loader/ChainLoaderFactoryTest.php Outdated
{
public function testImplementsLoaderFactoryInterface()
{
$rc = new \ReflectionClass('\Liip\ImagineBundle\DependencyInjection\Factory\Loader\ChainLoaderFactory');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might have asked this before but I wonder if these assertions on interface are still needed?

Copy link
Collaborator Author

@robfrawley robfrawley Jun 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

IDK if they are "needed" but for consistency with the other loader tests, I feel like we should include it for now. In the meantime, I have changed it to remove the reflection-based method used, with a much more sensible use of PHPUnit's assertInstanceOf() test assertion.

Tests/DependencyInjection/Factory/Loader/ChainLoaderFactoryTest.php Outdated
));

$this->assertArrayHasKey('loaders', $config);
$this->assertCount(2, $config['loaders']);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Worth asserting that the array itself is as we expect, versus the count?

Tests/Binary/Loader/ChainLoaderTest.php
private function assertValidLoaderFindReturn($return, $message = null)
{
$this->assertInstanceOf('\Liip\ImagineBundle\Model\FileBinary', $return, $message);
$this->assertStringStartsWith('text/', $return->getMimeType(), $message);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a change, just curiosity--Is this assertion purely for the case where something is returned?

Copy link
Collaborator Author

@robfrawley robfrawley Jun 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yup. I needed a clean way to validate the returned values from loaders, and checking if it is a FileBinary and a text file seemed pretty solid.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Very nice way of going about it, and a slightly better assertion IMO than just checking something is present.

@robfrawley robfrawley dismissed alexwilson’s stale review June 20, 2017 18:26

Resolved comments!

@robfrawley
Copy link
Collaborator Author

robfrawley commented Jun 20, 2017 •
edited
Loading

@antoligy Never tried to "dismiss" a review; wanted to see how that worked. ;-) Anyway, that was a crazy quick reviewing! How do you feel about the merits of this PR, as well as the implementation? Worth it? FYI: This still isn't ready for merge, as docs are still missing.

@robfrawley robfrawley added this to the 1.8.1 milestone Jun 20, 2017
@robfrawley robfrawley added Level: New Feature 🆕 This item involves the introduction of new functionality. State: Reviewing This item is being reviewed to determine if it should be accepted. labels Jun 20, 2017
@robfrawley robfrawley self-assigned this Jun 20, 2017
@robfrawley robfrawley force-pushed the feature-add-chain-data-loader branch from a9e6e92 to 6e8ec6d Compare June 20, 2017 19:37
@alexwilson
Copy link
Collaborator

Fair enough! I didn't say anything about the merits of this PR earlier: I think it's a very useful addition, and similar projects of this type have similar logic. For me it's particularly interesting in clustered environments where a system might have an asset available locally to it quicker than the same asset over a network. Will be interesting to hear/see other uses that may emerge too!

cedricziel
cedricziel approved these changes Jun 28, 2017
View reviewed changes
Copy link
Collaborator

@cedricziel cedricziel left a comment

Choose a reason for hiding this comment

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

Great new feature - thx Rob!

@cedricziel cedricziel merged commit 6e1be77 into liip:1.0 Jun 28, 2017
@robfrawley
Copy link
Collaborator Author

robfrawley commented Jun 29, 2017 •
edited
Loading

I'll open a new pull request with the documentation for this feature sometime tomorrow, as my comment about this fell through the cracks #953 (comment):

This still isn't ready for merge, as docs are still missing.

@cedricziel
Copy link
Collaborator

woops.

@robfrawley robfrawley mentioned this pull request Jun 29, 2017
Merged
@robfrawley
Copy link
Collaborator Author

@cedricziel It's all good; give #957 a review when you can.

@robfrawley robfrawley changed the title Add chain loader implementation [Data Loader] Add a chain loader implementation Jul 13, 2017
@robfrawley robfrawley mentioned this pull request Aug 31, 2017
Merged
@robfrawley robfrawley assigned maximgubar and unassigned maximgubar May 17, 2018
@robfrawley robfrawley mentioned this pull request May 17, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cedricziel cedricziel cedricziel approved these changes

@alexwilson alexwilson alexwilson approved these changes

Assignees

@robfrawley robfrawley

Labels
Level: New Feature 🆕 This item involves the introduction of new functionality. State: Reviewing This item is being reviewed to determine if it should be accepted.
Projects
None yet
Milestone
1.9.0
Development

Successfully merging this pull request may close these issues.
4 participants
@robfrawley @alexwilson @cedricziel @maximgubar