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

Move Group to src #4046

Merged
merged 8 commits into from Dec 10, 2017
Merged

Conversation

MrPetovan
Copy link
Collaborator

Related to #3878

This is my tangential work for #4041, it became so big I have to submit it now before I go further.

Runs with no errors.

Corresponding PR in addons to come.

@MrPetovan MrPetovan changed the title Move Group to Model Move Group to src Dec 9, 2017
if (intval($def_gid)) {
group_add_member($importer['uid'], '', $contact_record['id'], $def_gid);
}
Group::addMember(User::getDefaultGroup($importer['uid'], $contact_record["network"]), $contact_record['id']);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Before the change there was this if that checked whether the default group was set. Is this covered in Group::addMember now?

Copy link
Collaborator Author

@MrPetovan MrPetovan Dec 10, 2017

Choose a reason for hiding this comment

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

Yep, if the group id is invalid it doesn’t add anything.

@@ -289,7 +265,7 @@ function register_content(App $a) {
'$passwords' => $passwords,
'$password1' => array('password1', t('New Password:'), '', t('Leave empty for an auto generated password.')),
'$password2' => array('confirm', t('Confirm:'), '', ''),
'$nickdesc' => str_replace('$sitename',$a->get_hostname(), t('Choose a profile nickname. This must begin with a text character. Your profile address on this site will then be \'<strong>nickname@$sitename</strong>\'.')),
'$nickdesc' => t('Choose a profile nickname. This must begin with a text character. Your profile address on this site will then be \'<strong>nickname@%s</strong>\'.', $a->get_hostname()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice catch that t(...) already is doing the printf stuff.

*/
public static function getIdByName($uid, $name)
{
if ((! $uid) || (! strlen($name))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here you could omit some paranthesis, if you want to.

*/
public static function addMember($gid, $cid)
{
if (!($gid && $cid)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that some if (!$gid || !$cid) { is better to read than this line above.

'selected' => ''
]
];
foreach ($groups as $group) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

When you use dba::select you mustn't use a foreach to go through the result but a dba::fetch.

}

if (DBM::is_result($groups)) {
foreach ($groups as $group) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is the same like above. Additionally you can omit the is_result since the dba::fetch will handle it correctly if there is no result.

Copy link
Collaborator

@annando annando left a comment

Choose a reason for hiding this comment

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

There are some places where the database calls need to be fixed. Please have a look at other database calls in your PR as well - possibly I overlooked some of them.

Hypolite Petovan added 2 commits December 10, 2017 01:06
{
$o = '';

$stmt = dba::select('group', [], ['deleted' => 0, 'uid' => $uid], ['order' => ['name' => 'ASC']]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

'ASC' will not work as parameter. The system is expecting a boolean value (true for "DESC", false or no parameter at all for "ASC")

Sorry for spotting this just now. It was very early yesterday :-)

]
];

$stmt = dba::select('group', [], ['deleted' => 0, 'uid' => local_user()], ['order' => ['name' => 'ASC']]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is the same

Copy link
Collaborator

@annando annando left a comment

Choose a reason for hiding this comment

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

There still are two small dba issues.

@MrPetovan
Copy link
Collaborator Author

Here you go!

@annando annando merged commit 57f8496 into friendica:develop Dec 10, 2017
@zeroadam
Copy link

Just got this today, about 5 minutes ago, latest develop.
PHP Fatal error: Uncaught Error: Call to undefined method Friendica\\Model\\Group::create_member() in /srv/http/friendica/mod/group.php:177

@MrPetovan
Copy link
Collaborator Author

It should be addMemberByName instead.

@annando
Copy link
Collaborator

annando commented Dec 14, 2017

I just saw that it is wrong in api.php as well. There are two calls to Group::create_member as well. There it should be "addMember" I guess.

P.S: Are you looking at it or should I create a PR?

@MrPetovan
Copy link
Collaborator Author

Watch out, create_member directly maps to Group::addMemberByName.

If the group id is available, please use Group::addMember instead.

@MrPetovan
Copy link
Collaborator Author

I can’t fix it at the moment, knock yourself out.

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.

None yet

3 participants