`selected_by_autohighlight?` is broken for unevaluated route URLs

[stefan-kolb]

Sometimes route URLs of nested navigation items may not be evaluated at the runtime context of the parent navigation element. See this example where route URLs are only dynamically set inside the route contexts.

SimpleNavigation::Configuration.run do |navigation|
  navigation.items do |primary|
    primary.item :index, 'Home', '/home' do |index|
      index.item :vendor, @vendor_name, @vendor_path
    end
  end
end

Starting with version 4 of simple-navigation this example is broken. The problem should be introduced by commit 8669c5e98a7ddb5bb237894e3a6ac8b4da7beee2:

     def selected_by_autohighlight?
-      auto_highlight? &&
-      (root_path_match? ||
-       (url_without_anchor &&
-        SimpleNavigation.current_page?(url_without_anchor)))
+      return false unless auto_highlight?
+      root_path_match? ||
+      SimpleNavigation.current_page?(url_without_anchor) ||
+      autohighlight_by_subpath?
+    end

Removing the (url_without_anchor && SimpleNavigation.current_page?(url_without_anchor))) also evaluates Items with unevaluated URLs (nil) and crash on the SimpleNavigation.current_page?method when executing CGI::unescape.

Probably checking for nil routes again will resolve this problem, although I'm leaving this for discussion as the overall code has gotten quite nested and hard to oversee over time.

Let me know what you think.

[simonc]

Hi @stefan-kolb.

I just pushed a fix checking url, it should be all good now.

Feel free to reopen this issue if the problem persists :)

[stefan-kolb]

Alright, would be good if you release a new version including this soon as this is probably a deal breaker for v4

On another note: I've looked at vast parts of the overall code now and I think it includes way too much control flow logic done by logical operator chaining, e.g. url && template && template.current_page?(url). This makes it really hard to follow the control flow when multiple options are chained. Imho explicit ifs for exception cases etc. would help to avoid such bugs and make the code more readable.

[simonc]

All-in-all I agree but I'd like to push this a step further and re-think some parts of the code and decouple/simplify it. Sadly I don't have a lot of time at the moment. In the coming months I may have a bit more time to work on it ;)

[andi]

@stefan-kolb will release a patch version in the next 1-2 days...

Regarding the logical operator chaining: I agree that this should be simplified and as @simonc has stated, we'll do in the future. If you have any spare time in the meantime, feel free to simplify the code yourself and send PR's :-)

[andi]

4.0.2 has been released

[stefan-kolb]

@simonc I just took a look on your fix 12ebd2858fdf71c272de1216995b29372419717c. You are only fixing behavior for the Rails adapter. Sinatra and Padrino remain broken. We need to reintroduce the check in front of the adapters as far as I can tell, i.e. in def selected_by_autohighlight?. It's probably better if I submit a fix after verifying it is working for my scenario.

[simonc]

Alright ;)