Switch key and value in listing

[Eighke]

Hi,

In Version, the listing() function uses the name as key. But the name isn't unique and if you used a name twice or more, only the last is sent back. This also is a problem in Project.

Would it not be better to use the ID as key?

$ret[(int) $e['id']] = $e['name']; 

And for getIdByName()

public function getIdByName($name)
{
  $arr = array_flip($this->listing());

[wsuff]

Hmm. I haven't thought of this yet. Glad you raised the issue here at least for discussion. It sounds like it might be a wise idea. My only concern would be how do we warn users of the library of the change in behavior? I assume a change like this would require the user of the library to adjust their code to release the key is now the int id and not project name. Where else did you see this issue? It wasn't too clear to me from the post. Thanks for reporting it.

[kbsali]

Oh! That's a good point indeed. Although as it really happen to you before that you named 2 Versions or Projects the same way?
I'm not trying to argue your point, if the name is not unique then this indeed is a bug!

@wsuff : how about renaming the listing method "listingById()" and correcting all the listing() method with the changes proposed by @Eighke ? The change would then be almost transparent to anybody using the listing(). This is not a stable version anyway, so i would say this BC breaks are ok at such an early stage.

What do you think?

[Eighke]

@kbsali yes I have some sub Projects with the same name. Something like :

• Project 1
  • Frontend
  • Backend
• Project 2
  • Backend
  • Engine

@wsuff , well good question. I checked, only Version and Project is affected. In the other class the name is unique. listingById() isn't a bad idea or maybe something like :

public function listing($project, $forceUpdate = false, $reverse = true)
{
    if (true === $forceUpdate || empty($this->versions)) {
        $this->all($project);
    }
    $ret = array();
    foreach ($this->versions['versions'] as $e) {
        $ret[$e[(int) 'id']] =  $e['name'];
    }

    return $reverse ? array_flip($ret) : $ret;
}

Dunno which one is the best option.

[wsuff]

Ah. It makes more sense now. This wasn't a case I've noticed on my own install of Redmine yet since getting people to use a system after having none has been enough of challenge. =) listById() sounds ok to me. I can't really see any major downside to either proposed solution tho. Perhaps just a matter of preference and keeping consistence in the library.

Just realized this project is only month 1/2 old or so. Guess I lucked out that it was available when I went looking for one.

[kbsali]

@Eighke thanks for pointing out this solution,
i've implemented it for Version and Project as you mentioned these were the 2 affected classes!
Please review the changes and close if ok.

[Eighke]

Tested and approved. :)

Thanks.