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

Enable configuration of post processors using parameters #720

Closed
wants to merge 1 commit into from

Conversation

notrix
Copy link

@notrix notrix commented Mar 24, 2016

No description provided.

@darceee2
Copy link

👍

@alexwilson
Copy link
Collaborator

👍 Could play interestingly with this - #717

@shordeaux
Copy link

👍

@lsmith77 lsmith77 added the State: Reviewing This item is being reviewed to determine if it should be accepted. label Jun 19, 2016
@@ -31,6 +31,7 @@
"symfony/form": "~2.3|~3.0",
"symfony/console": "~2.3|~3.0",
"symfony/phpunit-bridge": "~2.3|~3.0",
"symfony/process": "~2.3|~3.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this necessary?

Copy link
Author

Choose a reason for hiding this comment

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

Post processors use ProcessBuilder class from symfony/process component. Maybe it should be included in composer require section?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah .. should be in the require section

@lsmith77
Copy link
Contributor

needs a rebase .. also a small entry to the docs is needed. sorry for the delay

@alexwilson
Copy link
Collaborator

alexwilson commented Jul 12, 2016

Separately to this, I wonder if it's worth applying the same default configuration to the other two PostProcessors, and also if it's worth implementing the ConfigurablePostProcessorInterface to these? More flexibility + discouraging maintaining state in the PostProcessor which should help with perf.

Happy to raise as an issue if so.

@lsmith77
Copy link
Contributor

sounds good to me ..

@lsmith77
Copy link
Contributor

there is overlap with #756 and this PR

@alexwilson
Copy link
Collaborator

alexwilson commented Jul 15, 2016

Looks like that's been taken care of in that PR, leaving only one item for the to-do list. 😄

@notrix If you haven't the time to write docs atm, I'm happy to write up some docs and PR against your local repo, leaving just the rebase against master before this can be merged - Sound okay?

EDIT: Ninja'd!

@notrix
Copy link
Author

notrix commented Jul 15, 2016

@antoligy it would be great if you could write some docs. Thnx

@alexwilson
Copy link
Collaborator

Done! notrix#1

@lsmith77
Copy link
Contributor

great .. github tells me that there are conflicts that must be resolved. could you rebase on master?

Fix for invalid variable name

Move symfony/process component to require section

Adding documentation for new parameters.

add @event annotation to let IDEs known event names and class instance

Introduce mozjpeg and pngquant post-processors, add transform options, and document.

Fix style issues.

Move runtime configuration to separate interface using

Fix tempnam usages

Quote strings starting '%' in YAML

Add Flysystem resolver

As a counterpart to the flysystem loader, using this resolver
it is also possible to store the cached filter-files in
flysystem adapters.

* add FlysystemResolver
* add tests for FlysystemResolver
* add documentation for FlysystemResolver
* exclude correct `Tests` folder from scrutinizer analysis

background filter: allow image positioning

allow positioning of the image on the background.
uses the same position options as the watermark filter.

Implement Imagine Grayscale filter

Add phpunit  tests for Grayscale filter

Change array definition for PHP 5.3 support

Add documentation for Grayscale filter

Style CI

Upgrade Imagine lowest dependency from 0.5 to 0.6.3 for Grayscale filter results purpose

next release will be 1.6.0

Applied fixes from StyleCI

Downscale filter scales an image to fit bounding box

Test for DownscaleFilterLoader

PHP 5.3 array style

Make StyleCI happy

Invalid exif orientation values should be a no op

Applied fixes from StyleCI
@notrix
Copy link
Author

notrix commented Jul 21, 2016

This is madness. I have merged with liip:master branch and got additional commit to this PR. So I rebased and now it shows 31 files modified.. Is it possible to fix this mess?

@alexwilson
Copy link
Collaborator

In this case it might be worth raising a second PR which resolves this one.

Ideally after rebasing you'd want to force push to your branch, but it
sounds like it might be a little late for that!

On 21 Jul 2016 8:24 am, "Vaidas" [email protected] wrote:

This is madness. I have merged with liip:master branch and got additional
commit to this PR. So I rebased and now it shows 31 files modified.. Is it
possible to fix this mess?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#720 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAa29D3LUdTGG00y23ODrsnLTwh8jV89ks5qXx7EgaJpZM4H35Jq
.

@lsmith77
Copy link
Contributor

@notrix generally you want to rebase feature branches from master instead of merging from master:
http://symfony.com/doc/current/contributing/code/patches.html#rebase-your-patch

question is if #756 covers everything in this PR?

@alexwilson
Copy link
Collaborator

#756 doesn't include the parameters, although has now introduced a merge conflict with the constructor. I'll address that in turn - For the meantime, I've rebased and have created #759.

lsmith77 added a commit that referenced this pull request Jul 22, 2016
Enable configuration of post processors using parameters (closes #720)
@lsmith77 lsmith77 removed the State: Reviewing This item is being reviewed to determine if it should be accepted. label Jul 22, 2016
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.

5 participants