[Scrutinizer] Fix issues in mod/ (_well_known to contactgroup) #4171

MrPetovan · 2018-01-05T01:28:58Z

Part of #4176
Follow-up to #4166

- Remove unused variables and parameter - Updated function documentation - Fix formatting - Add back uninitialized variables - Simplify nested conditions

annando

Please have a look at the change in functionality that I described.

annando · 2018-01-05T01:41:23Z

include/datetime.php

-		if (($dow == $f) && (! $started)) {
-			$started = true;
-		}
+		$started = (($dow == $f) && (!$started));


This is a difference in functionality. The original code would never set $started back to "false". This code is able to do so. Is it intended?

No, good catch, I went too fast on my formatting spree.

annando · 2018-01-05T01:45:11Z

mod/admin.php

@@ -720,7 +709,7 @@ function admin_page_summary(App $a)
 	if (Config::get('system', 'check_new_version_url', 'none') != 'none') {
 		$gitversion = Config::get('system', 'git_friendica_version');
 		if (version_compare(FRIENDICA_VERSION, $gitversion) < 0) {
-			$warningtext[] = sprintf(t('There is a new version of Friendica available for download. Your current version is %1$s, upstream version is %2$s'), $FRIENDICA_VERSION, $gitversion);
+			$warningtext[] = sprintf(t('There is a new version of Friendica available for download. Your current version is %1$s, upstream version is %2$s'), FRIENDICA_VERSION, $gitversion);


t() includes sprintf() doesn't it?

Yep, I'll simplify the call. I'm not concentrating on it because it isn't a code error per se.

annando · 2018-01-05T01:48:36Z

mod/admin.php

-					uninstall_theme($th['name']);
-					install_theme($th['name']);
-				}
+		foreach ($themes as $th) {


I haven't checked: Can we be sure that $themes is always an array?

Defined as such on line 1963, populated on line 1982.

annando · 2018-01-05T01:57:02Z

mod/cal.php

 			$remote_contact = true;
 		}
 	}
-	if(! $remote_contact) {
-		if(local_user()) {
-			$contact_id = $_SESSION['cid'];


Is this variable unused?

After line 129, no. If you think this call should be moved up let me know. However $remote_contact will always be false so I don't think it's warranted.

I meant the assignment of the variable with the content of $_SESSION['cid']; which now doesn't happen in the code any more if I see it right.

- Replaced sizeof by count - Used DBM::is_result()

- Use defaults for request parameters

annando

I don't understand the reason for one change.

annando · 2018-01-05T08:50:35Z

mod/admin.php

@@ -1266,7 +1266,7 @@ function admin_page_site(App $a)
 		'$banner'		=> array('banner', t("Banner/Logo"), $banner, ""),
 		'$shortcut_icon'	=> array('shortcut_icon', t("Shortcut icon"), Config::get('system','shortcut_icon'),  t("Link to an icon that will be used for browsers.")),
 		'$touch_icon'		=> array('touch_icon', t("Touch icon"), Config::get('system','touch_icon'),  t("Link to an icon that will be used for tablets and mobiles.")),
-		'$info'			=> array('info', t('Additional Info'), $info, sprintf(t('For public servers: you can add additional information here that will be listed at %s/servers.'), get_server())),
+		'$info'			=> array('info', t('Additional Info'), $info, t('For public servers: you can add additional information here that will be listed at %s/servers.', System::baseUrl()), get_server()),


Why had you added System::baseUrl()) here?

If you look at the translation string, there is a %s but no interpolation variable was provided. I just filled in the blank.

The %s belongs to get_server().

annando · 2018-01-05T08:52:59Z

mod/admin.php

@@ -2121,7 +2123,7 @@ function admin_page_themes(App $a)
 		'$function'            => 'themes',
 		'$plugins'             => $plugins,
 		'$pcount'              => count($themes),
-		'$noplugshint'         => sprintf(t('No themes found on the system. They should be paced in %1$s'),'<code>/view/themes</code>'),
+		'$noplugshint'         => t('No themes found on the system. They should be paced in %1$s', '<code>/view/themes</code>'),


annando · 2018-01-05T08:55:43Z

mod/admin.php

-	$nu_name     = (x($_POST, 'new_user_name')     ? $_POST['new_user_name']     : '');
-	$nu_nickname = (x($_POST, 'new_user_nickname') ? $_POST['new_user_nickname'] : '');
-	$nu_email    = (x($_POST, 'new_user_email')    ? $_POST['new_user_email']    : '');
+	$pending     = defaults($_POST, 'pending'          , array());


Just a style question: Wouldn't it be preferable to set the comma directly after the parameter and not with much space before?

To align with the followingdefaults() calls default values.

MrPetovan · 2018-01-06T00:35:27Z

mod/common.php

+		/// @todo : Initialize $profile_uid
+		$r = q("SELECT `id` FROM `contact` WHERE `nurl` = '%s' AND `uid` = %d LIMIT 1",
+			dbesc(normalise_link(get_my_url())),
+			intval($profile_uid)


@annando Any idea how to properly initialize $profile_uid here?

Isn't this the local_user() in most cases?

Maybe, but this is the module showing the common friends between two users/contacts, so there are many possibilities.

Common friends is done differently. But for a deeper look I'm too tired now.

That's okay, it's been broken like this for a while, it can wait a little longer.

Hypolite Petovan added 3 commits January 4, 2018 19:42

Fix Scrutinizer issues in mod [_well_know -> contactgroup]

819592c

- Remove unused variables and parameter - Updated function documentation - Fix formatting - Add back uninitialized variables - Simplify nested conditions

Fix parse error

837ac29

Additional issue fixes

6cfd3f5

annando requested changes Jan 5, 2018

View reviewed changes

Hypolite Petovan added 4 commits January 4, 2018 22:51

Revert assignment change in datetime

c544258

Simplify sprintf(t[t]()) calls

278068c

More issues fixed

392f66f

- Replaced sizeof by count - Used DBM::is_result()

Simplifying admin_page_users_post function

8f477c4

- Use defaults for request parameters

annando requested changes Jan 5, 2018

View reviewed changes

Hypolite Petovan added 3 commits January 5, 2018 08:30

Fix typo in mod/admin

4a52ceb

Merge branch 'develop' into task/fix-scrutinizer-issues

8d07a69

Fix variable confusion in admin

3883868

annando merged commit 045a0b2 into friendica:develop Jan 5, 2018

MrPetovan mentioned this pull request Jan 5, 2018

Scrutinizer error tracking issue #4176

Closed

MrPetovan commented Jan 6, 2018

View reviewed changes

MrPetovan mentioned this pull request Jan 6, 2018

[Scrutinizer] Fix issues in mod/contacts #4178

Merged

MrPetovan added this to the 3.6 milestone Mar 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Scrutinizer] Fix issues in mod/ (_well_known to contactgroup) #4171

[Scrutinizer] Fix issues in mod/ (_well_known to contactgroup) #4171

MrPetovan commented Jan 5, 2018 •

edited

annando left a comment

annando Jan 5, 2018

MrPetovan Jan 5, 2018

annando Jan 5, 2018

MrPetovan Jan 5, 2018

annando Jan 5, 2018

MrPetovan Jan 5, 2018

annando Jan 5, 2018

MrPetovan Jan 5, 2018 •

edited

annando Jan 5, 2018

annando left a comment

annando Jan 5, 2018

MrPetovan Jan 5, 2018

annando Jan 5, 2018

MrPetovan Jan 5, 2018

annando Jan 5, 2018

annando Jan 5, 2018

MrPetovan Jan 5, 2018 •

edited

MrPetovan Jan 6, 2018

annando Jan 6, 2018

MrPetovan Jan 6, 2018

annando Jan 6, 2018

MrPetovan Jan 6, 2018

[Scrutinizer] Fix issues in mod/ (_well_known to contactgroup) #4171

[Scrutinizer] Fix issues in mod/ (_well_known to contactgroup) #4171

Conversation

MrPetovan commented Jan 5, 2018 • edited

annando left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrPetovan Jan 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annando left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrPetovan Jan 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrPetovan commented Jan 5, 2018 •

edited

MrPetovan Jan 5, 2018 •

edited

MrPetovan Jan 5, 2018 •

edited