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

Use admin entry as service ID #3903

Merged
merged 1 commit into from
Jun 1, 2016

Conversation

wouterj
Copy link
Contributor

@wouterj wouterj commented Jun 1, 2016

Changelog

### Fixed
- Fixed `Pool::getAdminsByGroup()` for the new admin groups values

Subject

In df42796, the admin groups variable was
changed from containing only service IDs to containing an array of options
per item.

To get the service ID, the 'admin' entry should be used.

This is a bug in 3.0.0, but I can't see a way to target 3.0 specifically. Is there any way
to get this into a 3.0.1 versions or how do you handle that?

@greg0ire
Copy link
Contributor

greg0ire commented Jun 1, 2016

This is a bug in 3.0.0, but I can't see a way to target 3.0 specifically. Is there any way
to get this into a 3.0.1 versions or how do you handle that?

We simply don't :( (we know it would be better, but lack human resources for that).

@soullivaneuh
Copy link
Member

soullivaneuh commented Jun 1, 2016

we know it would be better

Not necessary. If we respect BC, patch can be included on a minor and user should not lock on 3.0.*.

So this will be applied on next release, 3.1 or 3.2. 👍

@wouterj more info: #3053

@soullivaneuh
Copy link
Member

@wouterj StyleCI is failing, could you please take a look?

In df42796, the admin groups variable was
changed from containing only service IDs to containing an array of options
per item.

To get the service ID, the 'admin' entry should be used.
@wouterj
Copy link
Contributor Author

wouterj commented Jun 1, 2016

So this will be applied on next release, 3.1 or 3.2. 👍

Ah, so this will be included in 3.1.1. Cool!

@wouterj StyleCI is failing, could you please take a look?

Should be fixed now

@soullivaneuh
Copy link
Member

Ah, so this will be included in 3.1.1. Cool!

Si is depending of the merged PR on 3.x since last release: https://github.com/sonata-project/SonataAdminBundle/pulls?utf8=%E2%9C%93&q=is%3Apr+base%3A3.x+merged%3A%3E2016-05-16T23%3A40%3A24Z+

In this case, minor PR are merged, so it will be 3.2.

I have to write more documentation about this. 👍

@greg0ire
Copy link
Contributor

greg0ire commented Jun 1, 2016

Not necessary.

Imagine you maintain three branches :

  • stable, that refuses anything new or any BC-break
  • unstable, that refuses any BC-break
  • testing

Adventurers will prefer using 4x, and people who need stability will use 3.0.1 rather than 3.1.1. When 3.2.0 is out, these people will move to 3.1.47, that will be stable AF. That's why I'm saying it is better: you can move from very stable to very stable, instead of moving from unstable to next unstable that is basically stabilized unstabled + new features that might introduce some instability.

Downside : you have to maintain 3 branches :/

@soullivaneuh
Copy link
Member

soullivaneuh commented Jun 1, 2016

and people who need stability will use 3.0.1 rather than 3.1.1

Why should they do this? This is a bundle, not a framework like symfony with a lot of component.

Again, if the BC is respected there is no need to lock on 3.0.

In any way, as you said, we have not enough resources to maintain minor branches. :-/

@greg0ire
Copy link
Contributor

greg0ire commented Jun 1, 2016

Why should they do this? This is a bundle, not a framework like symfony with a lot of component.

So what? The issue is the same for a framework or a tiny library : if you are not fixing a bug, you're introducing new code. If you're introducing new code, unless it's perfect, you probably introduce new bugs too. This is like the difference between Fedora and CentOS : either you want new things, or you want stability.

So in a debian-like setup like that, locking on 3.0 is good if you need absolute stability.

In anyway as you said, we have not enough resources to maintains minor branches. :-/

Not arguing that, but I'd like to make sure everyone fully understands what would be the point of a stable branch.

@soullivaneuh
Copy link
Member

if you are not fixing a bug, you're introducing new code. If you're introducing new code, unless it's perfect, you probably introduce new bugs too.

Globally you're right.

The issue is the same for a framework or a tiny library

Yes and no. For me, this depends of the size of the packages.

Take this one for example: https://github.com/nexylan/slack-bundle

This bundle currently just provide a service for the slack sdk. This would be cumbersome to maintain minor branches for that.

@soullivaneuh soullivaneuh merged commit c7e103c into sonata-project:3.x Jun 1, 2016
@soullivaneuh
Copy link
Member

Thank you @wouterj !

@wouterj wouterj deleted the fix-bug-admin-groups branch June 1, 2016 13:49
greg0ire pushed a commit to greg0ire/SonataAdminBundle that referenced this pull request Jun 2, 2016
In df42796, the admin groups variable was
changed from containing only service IDs to containing an array of options
per item.

To get the service ID, the 'admin' entry should be used.
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.

5 participants