It looks like Travis is complaining about 2 failed unit tests.

Looking at the comment just before the line you changed, it's actually intentional behavior. Also the failed test shows that it's the way it's supposed to work as it explicitely checks to remove that ending hyphen.

Well, yes, there is a comment there but it does not make any sense.

The code to take out the last hyphen is actually there for a long time and looks related to (deprecated?) grouped lists option.

However it was changed from an explode to a regex when the tests were introduced with #1706, but there is no indication why.
@Achal-Aggarwal do you remember why you changed that line?

Changing it back to $splitText = explode(' - ', $text, 2); fixes the issue, but breaks the tests as well.

From what I can recall I did it because according to comment https://github.com/Achal-Aggarwal/joomla-cms/blob/18ecae783fbaa4a7a4d55137a5bade558a8ee285/libraries/cms/html/select.php#L605 If there is a hyphen present in option's text and no data after it, we will just pick string before hyphen.

So if we have something like this https://github.com/joomla/joomla-cms/blob/staging/tests/unit/suites/libraries/cms/html/JHtmlSelectTest.php#L212 then in the earlier case we would have obtained option text back i.e no use of $splitText = explode(' - ', $text, 2); as the next if will be evaluated as true and spaces would have been added again.

IMO if we don't want to remove hyphen we can just remove the whole portion L654-L660.

I understand it that it should remove the hyphen if there is a space before and after it, But not when it's the last character (- Select Value - is fine). Looks like the regex does remove it always, which is be wrong.

I guess the code is needed for a deprecated way of creating grouped lists. But honestly I don't know how that worked back in 1.5.

@Achal-Aggarwal @tranduyhung Just stumbled over this again in my own code. Looks to me like either the tests or PR are wrong here. The current behavior definitively is wrong and needs fixing. A string with - Select Value - should be perfectly fine.

Ok, IMO to better understand what it should do lets have some testcases.
1. " foo " => " foo "
2. "- foo " => "- foo "
3. " foo- " => " foo- "
4. " foo - " => " foo"
5. " foo - " => " foo"
6. " foo -" => " foo"
7. " foo - a " => " foo - a "

On current implementation all cases are passing, but if test them on earlier one only 1,2,3 & 7 would pass.

Add an eight one
8. "- foo -" => "- foo -"
Currently that one becomes "- foo" which is certainly wrong.

That means you agree on first seven, :P then "- foo -" is a special case. We just need to incorporate this special one.

I'm not sure if the sixt one is correct. But I don't care if hyphen is stripped or not there.
The eigth one isn't a special case however.
There is actually only one special case, and that is when there is only one hyphen and that one is at the end surrounded by spaces. Then, and only then should it be stripped.
At least that's how I understand it

Edit:
4. "$$$$foo$-$$$$" => "$$$$foo" where $ represents space (' ')

So if you agree on 4th one also here is the fix + tests #3802

Looks like the $text is trimmed already so we can't check for following spaces. That may be the reason the original explode failed?
I think it may indeed be the easiest to threat it a bit special. I did this: https://github.com/Bakual/joomla-cms/compare/joomla:staging...Bakual:JHtmlSelect?expand=1 which looks promising so far.
What do you think?

After all 8th case is a special one. :D
we need to get rid of empty or else " foo - 0" gives " foo".
original explode fails because of " foo - " => " foo - "
But if every thing is based on first character to be "-" then yours one is also correct. I didn't about its specification that's why I acted & changed it based on the comment.

3802 is a general implementation, not focused on first char to be "-" though it passes 8th one also.

Yep, yours looks better than my attemp.

Closing this in favor of #3802