Piwigo Bugtracker

Piwigo bug tracker has moved to Github

This bugtracker is kept to provide history on old issues.


View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0003111Piwigousers & groupspublic2014.08.12 08:422014.10.01 16:24
Reporterpadawan 
Assigned Toplg 
PrioritynormalSeverityminorReproducibilityN/A
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version2.6.2 
Target Version2.8.0beta1Fixed in Version2.8.0beta1 
Summary0003111: Useless test block in include/functions_user.inc.php
DescriptionIn piwigo/include/functions_user.inc.php - starting at line 202, the whole IF block (7 lines) is totally useless. Whatever the conditions, the only thing that can happen is for $override to be set to numb, *again*.

   $override = null;
    if ($notify_admin and $conf['browser_language'])
    {
      if (!get_browser_language($override['language']))
      {
        $override=null;
      }
    }
TagsNo tags attached.
browserany
Database engine and version
PHP version
Web serverApache 1.3.x
Attached Files

- Relationships

-  Notes
(0007540)
padawan (reporter)
2014.08.12 08:43

I meant null not numb (thank you, autocorrect :-)
(0007640)
rvelices (developer)
2014.09.29 19:32

I see that function get_browser_language(&$lang)
takes a reference as parameter and it can fill it so is it really useless ?
(0007641)
plg (manager)
2014.09.29 22:46
edited on: 2014.09.29 22:51

I agree with rvelices but I understand why padawan strongly thinks this code is "useless".

Let's take an example.

$notify_admin = true;
$conf['browser_language'] = true;

=> if function get_browser_language finds a language in the browser which matches a language activated in Piwigo, then it sets $override['language'] to 'ru' (for example) and returns true.

After the if block, we have $override['language'] = 'ru', and $override is not null.

This code is a pain to read. If possible we should refactor it to make it simpler and more obvious.

(0007642)
svn (reporter)
2014.09.29 22:55

[Subversion] r29839 by plg on trunk

-----[Subversion commit log]----------------------------------------------------
bug 3111: add comment to help understand the code
(0007643)
padawan (reporter)
2014.09.30 08:49

No, if both conditions are true, then $override is set to null, which it is already. So, if these nested IF blocks are not useless, then there is a bug in this code since if you reduce it to only the variable assignment, $override is ALWAYS null. So either the first "$override = null" assignment is wrong, or the second one is.
(0007644)
rvelices (developer)
2014.09.30 09:01

padawan, would you mind giving it a try ? I'm not a php guru, but I think there is no bug and it works as it should
(0007645)
padawan (reporter)
2014.09.30 09:14

OK, j'ai eu l'explication de texte par un grand maître du PHP. Qui me dit que « ce code "tombe en marche" » et « entre le passage par ref et la création de tableau implicite, un dev me fait ça et je me fache rouge «. Il n'y a donc pas de bug, c'est juste difficile à lire.
(0007646)
plg (manager)
2014.09.30 09:18
edited on: 2014.09.30 09:19

OK, I've added some debug traces :

    $override = null;
    if ($notify_admin and $conf['browser_language'])
    {
      // if function get_browser_language finds a language in the browser
      // which matches a language activated in Piwigo, then it sets
      // $override['language'] to 'ru' (for example) and returns
      // true. $override won't stay null
      if (!get_browser_language($override['language']))
      {
        echo '< pre >$override 0 = '; print_r($override); echo '</ pre >';
        $override=null;
      }
    }

    echo '< pre >$override 1 = '; print_r($override); echo '</ pre >';
    
    create_user_infos($user_id, $override);

Here is the result:

$override 1 = Array
(
    [language] => fr_FR
)

It seems $override is not "ALWAYS null". Maybe the code is not written the best way. padawan, can you help us to write it a better way?

(0007647)
plg (manager)
2014.09.30 09:52

Would that be better? (more readable, easier to maintain)

    $language = null;
    if ($notify_admin and $conf['browser_language'])
    {
      get_browser_language($language);
    }

    $override = null;
    if (isset($language))
    {
      $override = array('language' => $language);
    }
(0007648)
svn (reporter)
2014.09.30 10:23

[Subversion] r29840 by plg on trunk

-----[Subversion commit log]----------------------------------------------------
bug 3111: cleaner code for get_browser_language (no more reference as parameter)

- Issue History
Date Modified Username Field Change
2014.08.12 08:42 padawan New Issue
2014.08.12 08:42 padawan browser => any
2014.08.12 08:42 padawan Web server => Apache 1.3.x
2014.08.12 08:43 padawan Note Added: 0007540
2014.08.13 09:16 plg Status new => assigned
2014.08.13 09:16 plg Assigned To => plg
2014.08.24 18:51 plg Target Version => 2.7.0RC2
2014.09.16 12:00 plg Target Version 2.7.0RC2 => 2.7.0
2014.09.24 09:49 plg Target Version 2.7.0 => 2.7.1
2014.09.29 19:32 rvelices Note Added: 0007640
2014.09.29 22:46 plg Note Added: 0007641
2014.09.29 22:51 plg Note Edited: 0007641
2014.09.29 22:51 plg Note Edited: 0007641
2014.09.29 22:55 svn Checkin
2014.09.29 22:55 svn Note Added: 0007642
2014.09.30 08:49 padawan Note Added: 0007643
2014.09.30 09:01 rvelices Note Added: 0007644
2014.09.30 09:14 padawan Note Added: 0007645
2014.09.30 09:18 plg Note Added: 0007646
2014.09.30 09:19 plg Note Edited: 0007646
2014.09.30 09:52 plg Note Added: 0007647
2014.09.30 10:23 svn Checkin
2014.09.30 10:23 svn Note Added: 0007648
2014.10.01 16:24 plg Status assigned => closed
2014.10.01 16:24 plg Resolution open => fixed
2014.10.01 16:24 plg Fixed in Version => 2.8.0beta1
2014.10.01 16:24 plg Target Version 2.7.1 => 2.8.0beta1


Copyright © 2000 - 2019 MantisBT Team
Contact
Powered by Mantis Bugtracker