Add Imagine Bundle Recipe #14

robfrawley · 2017-05-06T04:27:15Z

This pull request adds a Symfony Flex recipe for LiipImagineBundle, a popular image manipulation bundle that leverages the Imagine Library internally.

I am waiting for some reviews from my fellow bundle maintainers, so do not merge this immediately. Also, as there isn't much information or many examples on writing these recipes yet, I look forward to fielding reviews or change requests from this repository's maintainers, as well.

Hopefully, this bundle will land in the exciting Symfony Flex ecosystem shortly!

robfrawley · 2017-05-06T05:31:22Z

I'm left a little confused by the automated pull request checker, which has somehow determined that the liip/imagine-bundle package does not exist with any available versions, despite its existence with a plethora of available versions: what am I getting wrong here?

The following errors occurred while validating the pull request: package "liip/imagine-bundle" does not have any available version on Packagist

alexwilson · 2017-05-06T16:06:05Z

Just to check, is it possible that this is because of our requirement on ext-gd?

fabpot · 2017-05-06T17:06:21Z

There is a bug in the validator (the issue here is that there is no dev-master version on Packagist). I will fix it soon.

alexwilson · 2017-05-06T17:28:12Z

Ah, thanks @fabpot!

javiereguiluz · 2017-05-06T17:31:31Z

liip/imagine-bundle/1.7/post-install.txt

+    - Twig: <comment>{{ imagine_filter('my_thumbnails') }}</comment>
+    - PHP:  <comment>$this->get('liip_imagine.cache.manager')->getBrowserPath('/relative/path/to/image.jpg', 'my_thumbnails');</comment>
+
+  * <fg=blue>Read</> the documentation at <comment>http://symfony.com/doc/current/bundles/LiipImagineBundle</>


Please use https:// when linking to symfony.com. Thanks!

I will add a check for that :)

Fixed; thanks!

Pierstoval · 2017-05-06T17:55:03Z

liip/imagine-bundle/1.7/etc/routing/imagine.yaml

+    - GET
+  requirements: 
+    filter: "[A-z0-9_\\-]*"
+    path: .+


Maybe use 4 spaces instead of 2 to keep consistency with the rest of the codebase?

Pierstoval · 2017-05-06T17:55:11Z

liip/imagine-bundle/1.7/manifest.json

+    "copy-from-recipe": {
+        "etc/": "%ETC_DIR%/"
+    }
+}


Missing newline here

Fixed; thanks for the review!

Pierstoval · 2017-05-06T18:03:51Z

liip/imagine-bundle/1.7/etc/routing/imagine.yaml

+    methods: 
+        - GET
+    requirements: 
+        filter: "[A-z0-9_\\-]*"


Maybe use single quotes everywhere too? 😆 It'll help in backslash escaping too

Good idea; updated.

Koc · 2017-05-06T20:03:48Z

liip/imagine-bundle/1.7/etc/routing/imagine.yaml

@@ -0,0 +1,19 @@
+liip_imagine_filter: 


is it possible include @LiipImagineBundle/Resources/config/routing.xml instead of copy-pasting it content here?

My only concern is that the use of YAML here was an intentional choice made to lower the entry barrier for someone looking to, perhaps, edit the route paths; XML is generally more difficult for newcomers to follow or edit.

Assuming adding the following addition to the manifest.json results in the same auto-registration of the file, I agree, aside from my aforementioned comment.

"copy-from-package": { "Resources/config/routing.xml": "%ETC_DIR%/routing/" }

I agree that you could just use

liip_imagine: resource: '@LiipImagineBundle/Resources/config/routing.xml'

It's fairly enough to add routing config with flex

Good idea, except it doesn't allow for users to edit it easily.

so convert LiipImagineBundle routing to yaml and copy it?

That isn't something we can do until the next major release, as people rely on the current name for their own usage in pre-flex applications.

Let's move this discussion to liip/LiipImagineBundle#924 so as to not pollute the Flex maintainers with our internal discussions.

robfrawley · 2017-05-06T20:54:18Z

@fabpot Is there a standard for enabling multiple package versions and avoiding duplication of files; every single recipe file for this bundle is exactly the same for both version 1.7 and 2.0.

For example, is the solution found at the following link acceptable, where I created a .default directory with all the recipe files and then symlinked ./1.7 and ./2.0 to ./.default/?

github.com/robfrawley/recipes-contrib/tree/feature-include-version-1.x-and-2.x/liip/imagine-bundle

Is this a suitable strategy, or is there an alternative standard, or should the files simply be duplicated in each version folder?

Pierstoval · 2017-05-06T21:04:26Z

@robfrawley Take example of the PHPUnit recipe: https://github.com/symfony/recipes/blob/master/phpunit/phpunit/4.7/manifest.json

You can specify other versions that are compatible with this recipe.

But if files change, you'll have to create a new directory with a new manifest

robfrawley · 2017-05-06T21:07:20Z

@Pierstoval Oh, wonderful; I don't know how I missed that. Thanks!

robfrawley · 2017-05-07T22:13:29Z

Please do not merge until ~~liip/LiipImagineBundle#925 is merged and~~ ~~the newest release version, that is compatible with this recipe, 1.8.0 (see liip/LiipImagineBundle/pull/928) is released~~; will update once we're ready to go on our end. Update: we're ready to go!

fabpot · 2017-05-08T16:46:26Z

Bug fix, validation does not pass now because the 1.8 version does not exist yet, which is expected.

robfrawley · 2017-05-11T21:03:25Z

@fabpot You were right; I had thought the composer configuration was global, not per project; thanks for the clarification on that. The new PR deployment page is great, BTW.

At this point, this looks good from our end. I was able to successfully test both version 1.8 and 2.0. Let me know if you require any additional changes.

stof · 2017-05-12T08:48:54Z

@robfrawley extras are not global

robfrawley · 2017-05-12T23:21:33Z

~~Does anyone following this thread want to approve this (other than me) now that auto-merging is enabled? I love that the contrib repository is user-moderated now! See #21 for additional information.~~

fabpot · 2017-05-12T23:27:29Z

@robfrawley I would prefer to be the one who will approve this PR, the rules are still changing a bit, so better to wait a bit :)

robfrawley · 2017-05-12T23:33:18Z

@fabpot Apologies; I misunderstood the updated README.rst and didn't realize the changes were still in "flux". No worries; take your time! We're all just excited for Flex!

fabpot · 2017-05-13T02:06:59Z

@robfrawley I think everything is ready now. So, for that PR to be merged, a Symfony Core team member or a member of the Liip organization must approve the PR (the member must be publicly listed on the Liip organization).

robfrawley · 2017-05-13T02:10:44Z

@lsmith77 Care to approve this PR for our Symfony Flex recipe? Thanks @fabpot for all the hard work!

robfrawley · 2017-05-16T20:09:03Z

Hm; thanks for giving that a try @Pierstoval 🥇

fabpot · 2017-05-23T05:36:22Z

liip/imagine-bundle/1.8/post-install.txt

+
+  * <fg=blue;options=bold>Read</> <fg=blue>the documentation and</> <fg=blue;options=bold>ask</> <fg=blue>for help:</>
+    - Documentation: <comment>https://symfony.com/doc/current/bundles/LiipImagineBundle</>
+    - Issue Tracker: <comment>https://github.com/liip/LiipImagineBundle/issues</>


This block is way too long. It should not replace proper documentation. It should only be used to indicate the immediate next steps to make it work, if any.

Brought it down from 18 to 10 lines: is that more appropriate?

symfony-bot · 2017-05-23T15:56:12Z

liip/imagine-bundle/1.8/manifest.json

+    "copy-from-recipe": {
+        "etc/": "%ETC_DIR%/"
+    },
+    "version_aliases": ["2.0"]


Unsupported key "version_aliases"

fabpot · 2017-05-24T06:11:57Z

liip/imagine-bundle/1.8/post-install.txt

+
+  * <fg=blue;options=bold>Read</> <fg=blue>the documentation and</> <fg=blue;options=bold>ask</> <fg=blue>for help:</>
+    - Documentation: <comment>https://symfony.com/doc/current/bundles/LiipImagineBundle</>
+    - Issue Tracker: <comment>https://github.com/liip/LiipImagineBundle/issues</>


The last paragraph should be removed.

robfrawley force-pushed the master branch 4 times, most recently from db57f16 to 782f8e9 Compare May 6, 2017 05:20

robfrawley mentioned this pull request May 6, 2017

Symfony Flex Recipe liip/LiipImagineBundle#924

Closed

javiereguiluz reviewed May 6, 2017

View reviewed changes

robfrawley force-pushed the master branch 3 times, most recently from 6d8b53e to 910ac96 Compare May 6, 2017 17:54

Pierstoval suggested changes May 6, 2017

View reviewed changes

robfrawley force-pushed the master branch from 910ac96 to e61fa8e Compare May 6, 2017 18:00

Pierstoval reviewed May 6, 2017

View reviewed changes

robfrawley force-pushed the master branch 2 times, most recently from 19e1408 to 9b26ae4 Compare May 6, 2017 18:10

Koc reviewed May 6, 2017

View reviewed changes

robfrawley force-pushed the master branch 3 times, most recently from be14e01 to 8bb6d4b Compare May 7, 2017 22:11

symfony-bot deployed to staging May 8, 2017 16:27 View deployment

symfony-bot deployed to staging May 8, 2017 16:39 View deployment

robfrawley mentioned this pull request May 8, 2017

Release 1.8.0 liip/LiipImagineBundle#928

Merged

robfrawley changed the title ~~[WIP] Add Imagine Bundle Recipe~~ Add Imagine Bundle Recipe May 11, 2017

robfrawley force-pushed the master branch from 3ce81aa to fea58c9 Compare May 12, 2017 23:19

symfony-bot deployed to staging May 12, 2017 23:19 View deployment

symfony-bot previously approved these changes May 12, 2017

View reviewed changes

fabbot-io approved these changes May 13, 2017

View reviewed changes

robfrawley mentioned this pull request May 16, 2017

Add a note about PRs being merged automatically #21

Merged

Pierstoval approved these changes May 16, 2017

View reviewed changes

fabpot requested changes May 23, 2017

View reviewed changes

robfrawley dismissed symfony-bot’s stale review via 20a44a7 May 23, 2017 15:56

robfrawley force-pushed the master branch from fea58c9 to 20a44a7 Compare May 23, 2017 15:56

symfony-bot suggested changes May 23, 2017

View reviewed changes

robfrawley force-pushed the master branch from 20a44a7 to dcad3e5 Compare May 23, 2017 15:58

symfony-bot deployed to staging May 23, 2017 15:58 View deployment

symfony-bot previously approved these changes May 23, 2017

View reviewed changes

fabpot requested changes May 24, 2017

View reviewed changes

add liip/imagine-bundle recipe

d332467

robfrawley dismissed symfony-bot’s stale review via d332467 May 24, 2017 07:50

robfrawley force-pushed the master branch from dcad3e5 to d332467 Compare May 24, 2017 07:50

symfony-bot deployed to staging May 24, 2017 07:50 View deployment

symfony-bot approved these changes May 24, 2017

View reviewed changes

fabpot approved these changes May 24, 2017

View reviewed changes

symfony-bot merged commit 60e6d43 into symfony:master May 24, 2017

Add Imagine Bundle Recipe #14

Add Imagine Bundle Recipe #14

Conversation

robfrawley commented May 6, 2017

robfrawley commented May 6, 2017 • edited Loading

alexwilson commented May 6, 2017

fabpot commented May 6, 2017 • edited Loading

alexwilson commented May 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Koc May 6, 2017 • edited Loading

Choose a reason for hiding this comment

robfrawley May 6, 2017 • edited Loading

Choose a reason for hiding this comment

Pierstoval May 6, 2017 • edited Loading

Choose a reason for hiding this comment

robfrawley May 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley May 6, 2017 • edited Loading

Choose a reason for hiding this comment

robfrawley commented May 6, 2017

Pierstoval commented May 6, 2017 • edited Loading

robfrawley commented May 6, 2017

robfrawley commented May 7, 2017 • edited Loading

fabpot commented May 8, 2017

robfrawley commented May 11, 2017

stof commented May 12, 2017

robfrawley commented May 12, 2017 • edited Loading

fabpot commented May 12, 2017

robfrawley commented May 12, 2017 • edited Loading

fabpot commented May 13, 2017

robfrawley commented May 13, 2017 • edited Loading

robfrawley commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley commented May 6, 2017 •

edited

Loading

fabpot commented May 6, 2017 •

edited

Loading

Koc May 6, 2017 •

edited

Loading

robfrawley May 6, 2017 •

edited

Loading

Pierstoval May 6, 2017 •

edited

Loading

robfrawley May 6, 2017 •

edited

Loading

robfrawley May 6, 2017 •

edited

Loading

Pierstoval commented May 6, 2017 •

edited

Loading

robfrawley commented May 7, 2017 •

edited

Loading

robfrawley commented May 12, 2017 •

edited

Loading

robfrawley commented May 12, 2017 •

edited

Loading

robfrawley commented May 13, 2017 •

edited

Loading