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

Backport 2.6 compat to 0.3 + we need a tag #747

Merged
merged 3 commits into from May 4, 2015
Merged

Conversation

weaverryan
Copy link
Contributor

Hi guys!

This is a cherry-pick of sha: 9186f92, which was applied to the master branch only. The issue is that we need a new 0.3 tag, so I'm backporting this for that reason.

This will be the only commit that's on the 0.3 branch, so tagging it should be easy.

But, master has a lot of commits - @stloyd what's the plan for the 0.4 release?

Thanks!

@soullivaneuh
Copy link
Collaborator

@weaverryan Seems to broke lot of tests, can you please take a look?

weaverryan and others added 3 commits May 3, 2015 16:37
…ws null for these values, which makes them optional (basically, it gives 2.6 the same behavior with these classes as previous Symfony versions)
…ndefinedOptionsException

To be compatible with all versions, I'm expecting the more generic InvalidArgumentException.
If we ever only support 2.6+, this can be changed to UndefinedOptionsException, but I don't
see much harm here (we're just testing that validation for these options is setup correctly).
@weaverryan
Copy link
Contributor Author

@soullivaneuh Ah yes, thank you for telling me!

This should now be ready to merge (and tag!). Summary:

  1. Any optional options in "ResourceOwner"s that call setAllowedValues have had null added to the allowed values. This makes the bundle work identically on 2.6 as other versions.

  2. The exception that's thrown when you have an invalid option changed in 2.6, so I updated the tests to the base exception class that'll make the tests pass on 2.6 or before: weaverryan@c5c5530

The tests are PASSING - they're only failing because Composer failed to download for some reason on the 5.3.3 tests. I would love to see this get merged and tagged so that the stable version of the bundle can be used with Symfony 2.6.

Thanks!

@soullivaneuh
Copy link
Collaborator

Big deprecation cleanup is already on progress on #773, but for 0.4.

After taking a look on the README file, seemd 0.3 version only support Symfony <2.4.

So I'm not sure this PR should be merged. Let's @stloyd decide.

@weaverryan You can still use the 0.4.x-dev branch. I'm doing my best to help maintainers and, if possible, be contributor too in order to push a stable release soon.

Regards

@weaverryan
Copy link
Contributor Author

@soullivaneuh Aren't there BC breaks on 0.4 (master) versus 0.3? The issue now is that 0.3.7 is the latest tag and the 0.3 composer.json is compatible with 2.1 and higher.

This means that everyone starting the project today using composer require hwi/oauth-bundle will get 0.3 (even on Symfony 2.6). In other words, a lot of people - probably most people (including me) have this release, but it has some "quirks" (mostly works) on Symfony 2.6. By telling them to use 0.4 instead, we're forcing them to break BC in their app and telling them to upgrade to a version without a release anyways.

Phew, so what I'd love is:

  1. Merge this to keep 0.3 working. If there are other bugs, we can merge those int 0.3 to keep it alive and happy (but not new features).

  2. Get a release for 0.4 done as quickly as possible. When it's ready, give people clear upgrade instructions (I can help with this).

Thanks guys!

@soullivaneuh
Copy link
Collaborator

The issue now is that 0.3.7 is the latest tag and the 0.3 composer.json is compatible with 2.1 and higher.

Certainly a mistake as documentation is clear about it: "0.3 with support for Symfony >=2.1,<2.4"

composer.json as just not be updated. This should be done (ping @stloyd).

By telling them to use 0.4 instead, we're forcing them to break BC in their app and telling them to upgrade to a version without a release anyways.

With the big lack of update and many PR waiting, this would be more simpler to make a fresh 0.4 version quickly and officially abandon 0.3 support (with composer.json update).

Personally, I got issue too for 0.3 with SF 2.6. I upgraded this bundle to master without lot of pain.

But I agree, a stable release is needed from a long time. I'm trying to make things moving about it, see (and support :-) ) #769.

stloyd added a commit that referenced this pull request May 4, 2015
Backport 2.6 compat to 0.3 + we need a tag
@stloyd stloyd merged commit 001b024 into hwi:0.3 May 4, 2015
@stloyd stloyd added the Bugfix label May 4, 2015
@weaverryan
Copy link
Contributor Author

Thanks @stloyd! And I see you created a tag, and I agree that hopefully this is the last release for 0.3 :).

@soullivaneuh Thanks for pushing this bundle forward - I think it's a very very useful bundle and solves a very common need. It's one of the bundles that I would like to see get very polished, because I think a lot of people find it useful and a lot more people will find it useful in the future.

Thanks!

@weaverryan weaverryan deleted the 2.6-compat branch May 4, 2015 13:05
@soullivaneuh
Copy link
Collaborator

@weaverryan You're welcome!

It's a little bit painful to move a project to another namespace. Add contributors would be the best and easier way. 👍

@XWB
Copy link
Member

XWB commented May 4, 2015

I'd be happy to add 1 or 2 contributors. Any volunteers?

@stloyd
Copy link
Collaborator

stloyd commented May 4, 2015

@XWB I was talking with @asm89 about new contributors some time ago, and IIRC we agreed that before accepting new contributor we should see some PRs etc. being proposed, reviewed etc. from such person before decision about adding or not. Same way I was added to this bundle ;)

@soullivaneuh
Copy link
Collaborator

@XWB Any volunteers?

I propose my help. 👍

we agreed that before accepting new contributor we should see some PRs etc. being proposed, reviewed etc. from such person before decision about adding or not

I understand and agree.

By talking about that, big PR is nearly ready here: #773

@XWB
Copy link
Member

XWB commented May 4, 2015

@stloyd I fully agree. I was not planning to randomly add people to the project :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants