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

[1.0][filter] add ability to configre filters at runtime. #309

Conversation

makasim
Copy link
Collaborator

@makasim makasim commented Jan 30, 2014

No description provided.

@makasim
Copy link
Collaborator Author

makasim commented Jan 31, 2014

@havvg ping

1 similar comment
@makasim
Copy link
Collaborator Author

makasim commented Feb 3, 2014

@havvg ping

@makasim
Copy link
Collaborator Author

makasim commented Feb 5, 2014

@havvg what about this PR?

@havvg
Copy link
Contributor

havvg commented Feb 6, 2014

Hm.. I can't find the issue right now, I remember parts of it, but I'm not 100% sure. What's the use case behind this?

@makasim
Copy link
Collaborator Author

makasim commented Feb 6, 2014

What's the use case behind this?

@havvg see #313

@makasim
Copy link
Collaborator Author

makasim commented Feb 6, 2014

Hm.. I can't find the issue right now, I remember parts of it, but I'm not 100% sure.

I think you are confused it with another PR (#288). this one is a new one

'size' => array(300, 100)
)
)
));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@havvg it is much simpler to configure filters at runtime now. see the doc

Copy link
Contributor

Choose a reason for hiding this comment

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

But this example replaces the thumbnail filter (or parts of its configuration).
If you actually would do this with variables, the cached images are most probably not of the same filter (with the same configuration).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@havvg no it does not, it just merge configs return array_replace_recursive($this->filters[$filter], $runtimeConfig);

if call $this->filterManager->applyFilter($binary, $filter) next time it will return statically configured option.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@havvg maybe array_replace_recursive has to be moved from FilterConfiguration to FilterManager. I will do that.

@havvg
Copy link
Contributor

havvg commented Feb 6, 2014

I believe I mixed it up with dynamic filters, I can't see any needs for this right now. I need to think about this a bit a more :-)

I was very busy the other days, I want to complete the data remodelling these days, so the injection of filters becomes easier and we got a clean data structure.

@makasim
Copy link
Collaborator Author

makasim commented Feb 6, 2014

I believe I mixed it up with dynamic filters, I can't see any needs for this right now.

my PR about dynamic filters is based on this one. Also dynamic filters were requested several times on issues and even there were attempts to implement it.

If you are agree with interface we can merge it and change implementation later

<?php

$filterManager->filterApply($path, $filter, array $runtimeConfig = array());

@havvg
Copy link
Contributor

havvg commented Feb 6, 2014

I like the idea of dynamic filters and I'm aware this one is a required feature. But the way a filter becomes dynamic should not be modifying a configured one. A dynamic filter isn't configured, but it's injected at runtime.

The filter handling of this bundle should not be aware of any distinction between a configured filter (e.g. via bundles configuration) or a "dynamic" filter which for example is injected into the filter manager using a listener on kernel.request (route options => filter) or an actual runtime filter (user land code actually creating a filter prior using it).

So any changes required to make this happen, which are solely to make it possible to modify (add, re-configure, ..) a filter at runtime, are bad, imho. That's why I started to re-model the data structure, which will open many ways to append filters to the manager (incl. runtime, database backed etc.).

@makasim
Copy link
Collaborator Author

makasim commented Feb 6, 2014

A dynamic filter isn't configured, but it's injected at runtime.

that's how it is done, pre configured filters never modified.

@makasim
Copy link
Collaborator Author

makasim commented Feb 6, 2014

Configs merged at runtime if you pass it, if not pre-configured will be used.

The filter handling of this bundle should not be aware of any distinction between a configured filter (e.g. via bundles configuration) or a "dynamic" filter which for example is injected into the filter manager using a listener on kernel.request (route options => filter) or an actual runtime filter (user land code actually creating a filter prior using it).

Sounds a bit complected do we really need such abstractions?

@makasim
Copy link
Collaborator Author

makasim commented Feb 8, 2014

@havvg moved configs merge logic to FilterManager. Previously it was in FilterConfiguration::get method. Also I introduced a new method FilterManager::apply. It takes binary and config. You can apply filters even without having one configured in config.yml. apllyFilters reuse it.

Use cases:

// pre configured filter
$filterManager->applyFilter($binary, $filter);

// pre configured filter + partially configured at runtime
$filterManager->applyFilter($binary, $filter, array(
    'filters' => array(
        'thumbnail' => array('size' => array(100, 100))
    )
));

// at runtime
$filterManager->apply($binary, array(
    'filters' => array(
        'thumbnail' => array(
            'size' => array(100, 100),
            'mode' => 'outbound',
        )
    )
));

Pre configured filter config does not changed in any given example above

@makasim
Copy link
Collaborator Author

makasim commented Feb 13, 2014

@havvg ping. this is the current approach: #309 (comment)

@makasim
Copy link
Collaborator Author

makasim commented Feb 17, 2014

@havvg ping

@makasim
Copy link
Collaborator Author

makasim commented Feb 18, 2014

closing in favor #313

@makasim makasim closed this Feb 18, 2014
@makasim makasim deleted the filter-manager-runtime-configuration branch February 18, 2014 14:45
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.

2 participants