-
Notifications
You must be signed in to change notification settings - Fork 168
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
MINOR: restrict the available authors, because right now any Member c… #641
base: master
Are you sure you want to change the base?
Conversation
…an be an author which causes big slowdowns for edge case only see: silverstripe#640
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable enough to me - how easy is it to revery this via the config if you did want all members shown? I think that should probably be added to the docs
You might argue that, if your site has lots of members, it's easier to make this tweak via config than it is to undo this tweak for a smaller site where you do want all users in the dropdown - though I agree that's quite an unlikely scenario |
My 2c: IMO this is a breaking change, if a minor one. Given that this is already a configuration option, it's probably best left as is. It can be set to a given group for any projects that need it already and if setting that one piece of configuration is becoming a real chore, you could always make a very small module which has Silverstripe/blog as a dependency and sets your opinionated default, and require that for your projects instead of the blog module directly. |
Given this is targeting the master branch, any concerns about breaking changes aren't too important and this change should be considered on merit. Personally, I think having authors that can't even access the CMS doesn't make much sense, if that's a specific use-case of a site, then having to tweak this to allow all members is pretty trivial. I think my requests on this PR would be:
|
Fair point, I hadn't noticed it was targeting master. |
If Silverstripe is going to survive we need a more efficient way to add these changes. This was raised in March last year. I know that you, @GuySartorelli and @dhensby, are some of the few that actually are championing Silverstripe, but if you have a chance to raise this with your teams then please do so because this is a pretty trivial change and not adding it would add a lot of hassle to sites with many members and a blog ... |
I appreciate the frustration. There are lots of factors at play here, the main two being capacity and priorities. I know this kind of delay affects all our repositories, but more so with the fringe repos (Vs the core repos). In this instance, unfortunately the blog module isn't a priority because its usage is simply diminishing and we can see that as the external contributions fall off. I also think this change is low impact and priority because it's trivial to change via config in sites with a large number of users. I do think updates to changelogs and docs are important for behaviour changes like this, tests where there's no existing coverage, less so. But whilst these changes may seem trivial in isolation if there are lots of them that result in upgrade pain for consumers we need to mitigate that with clear docs and reasoning. |
…an be an author which causes big slowdowns for edge case only
see: #640