On the first and last tickets one of @prev_issue_id and @next_issue_id is nil but issue_path is called with the nil value triggering the Journey issue. The reason it didn't fail in 3.2.3 is because of #6196 - it discarded the nil positional arg and pulled the :id from the recall path parameters. I'm sorry, you'll have to fix the template - this works for me:

Ouch, this breaks any occurance of link_to_if .... something_path(possibly_nil). It used to be a useful idiom to have a link to a resource if the resource existed or a "disabled" link if not. Broke one of my own apps too.

@dgm unfortunately there's not a lot you can do - the url helper is called in the params before the condition inside link_to_if is checked. Rather than using the url helper you can use a hash or just pass the model if the polymorphic route is the right route, e.g:

The latter is nicer, but you need to fetch the model and not just the id. Personally I'd wrap the logic up inside a helper or decorator to simplify the view - there's too much going on inside that Redmine template.

Yeah, I can see that. An unfortunate side effect that worked before. But I agree, that path function should not be executed if there is no id, so I'll have to change a few places. Just saying, redmine probably isn't the only place this happened. :)

@pixeltrix that change in #6196 was a regression, and intentional / supported functionality. I think we should restore the previous behaviour for the next 3.2.x release and perhaps leave it off in master.

As you can see most of the time it generates a routing error - the only instances where it generates a route is when there's a single nil argument and then it generates an incorrect url unless it's to the current brand. It doesn't pull :id from the recall when you have at least one truthy argument - the positional argument wins when all the hashes are merged in url_for. However leaving out the positional arg still works:

The Redmine template example is just wrong - in 3.2.3 the previous/next links would be to the current issue and not to the previous/next issues, it's just that the link_to_if then discards the link. If the template had a nested url helper and the last argument was nil then it would get the same result in 3.2.3 and 3.2.5.

TL;DR all the fix in #6196 does is make the treatment of nil positional arguments consistent - if you think that nil positional arguments should pull from the recall parameters then that's a different argument - no pun intended. :-)

These errors have been coming to light after @tenderlove changed the behavior of nil/false arguments in Journey 1.0.2 - previously the nil argument would've been converted to empty string, whereas now it raises an error. If you revert #6169 then you'll probably want to revert the Journey fix as well, however I don't think I've see an example yet where a app no longer generates a correct url - generally we're just raising an error now instead of generating an incorrect url.

@NZKoz just be clear, replacing nil positional arguments from the recall parameters should only go in master as it's a change from existing behavior.

Also if you pass a new record then it will get pass the args.any? check but then raise an error since to_param on new record is nil. So in 3.2.3, brand_path(Brand.new) generates an error but brand_path(Brand.new.id) would pull the id from the recall parameters.

@pixeltrix I see, I missed the part where link_to_if was obviously going to never append the link if the value was nil / set to something incorrect so the nonsense-urls generated in this case were never seen.

So the journey fix was for #4587, which does indeed seem somewhat crazy. Did this functionality work in 3.0 or earlier?

@darix seems like in this case it's a regression, but we just changed what was 'undefined' behaviour to raise an error, I'm a little less strident on reverting this change unless there's another usecase that breaks?

Even though it slightly irks me that it feels like a regression, in this case the behavior was a throw-away. But if the original bug was to somehow happen in a place that isn't a throw-away, a more serious error could happen. I think it is better to leave it fixed, rather then allow undefined behavior.

This also serves as a warning to the dangers of "link_to_if"... it's not truly an if statement that avoids execution. I think it's a subtle trap to expect it to work the same as link_to(...) if condition?