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

Not overriding set models. #207

Merged
merged 1 commit into from
Oct 21, 2015
Merged

Not overriding set models. #207

merged 1 commit into from
Oct 21, 2015

Conversation

willtp87
Copy link
Member

No description provided.

@@ -560,7 +560,7 @@ function xml_form_builder_update_object(AbstractObject $object, array $associati
if ($label) {
$object->label = $label;
}
$object->models = array($association['content_model']);
$object->models = array_merge($object->models, array($association['content_model']));
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't array_unique or the like be better to avoid duplicating content-models? Or is there a use case for having multiple of the same content-model in here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is handled at a lower level.

Copy link
Member

Choose a reason for hiding this comment

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

For clarity and posterity, where is it handled?

Copy link
Member Author

Choose a reason for hiding this comment

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

jordandukart added a commit that referenced this pull request Oct 21, 2015
@jordandukart jordandukart merged commit d637f40 into Islandora:7.x Oct 21, 2015
@ruebot
Copy link
Member

ruebot commented Oct 21, 2015

I think it would have been extremely helpful to have not merged until @DiegoPino provided some input here since he is the component manager.

@DiegoPino
Copy link
Contributor

@willtp87 and @jordandukart. It's time to discuss some open issues that require the whole community attention.

Most of us contribute to this community with the sole intention of making Islandora a better software. We all join each week in long discussions on what/how to fix problems detected in the stack and most of the time i see people getting their pulls and code reviewed and even sometimes rejected. This is good because it's a group decision. People act accordingly and always follow the group's suggestions and also your view on the stack because you have a deep knowledge on this. We all learn. We grow.

That said, a better software it's not only build on good code, also on strong community. And community stays working for us and for you, when there is respect and common rules that apply to everyone.
And you guys are not following the rules nor being respectful. I can't even imagine a situation where someone X and me (me friend of X) mutually review pulls to, e.g Tuque, and then merge without asking you or the rest. Even less closing an JIRA issue without giving the enough time to peer-review what was done, more if the reporter is someone else, who took the time to find, describe and identify a problem.

If you look closely, the whole reason for having this pull is because you have been doing this systematically before. Unit tests are good, but external human review is even better because interaction between modules requires real UI testing sometimes.

I don't know if you are aware of this, but every time you add new code to the stack without discussing, merge without asking at least the maintainer's opinion, you are sub-estimating the community, not only a particular person.

Your pull is correct in terms of code so i won't reject this merge. But the way you are acting is not right and the consequences are many, one of them is introducing bugs, this is not the first time, but the most important and dangerous one is making the community weak.

@whikloj
Copy link
Member

whikloj commented Oct 21, 2015

👍 to @DiegoPino's comments. These type of changes make it much harder to work with the project and begs the question of how much hassle is Islandora worth.

@ruebot
Copy link
Member

ruebot commented Oct 21, 2015

👍 to @DiegoPino and @whikloj's comments as well. This continued behavior that is antagonistic to community developers is alienating, and destructive to the project. This is an example of why I am not serving as release manager after this release. It just isn't worth the frustration anymore.

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