Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid null exception when href is null. #15765

Merged
merged 6 commits into from
Apr 22, 2024
Merged

Avoid null exception when href is null. #15765

merged 6 commits into from
Apr 22, 2024

Conversation

MikeAlhayek
Copy link
Member

Fix #15752

@Piedone
Copy link
Member

Piedone commented Apr 15, 2024

As I explained under the issue, that property shouldn't be null, to begin with. And if it is, we should throw a proper exception, because it's an invalid state for the menu, we shouldn't hide that.

@MikeAlhayek
Copy link
Member Author

As I explained under the issue, that property shouldn't be null, to begin with. And if it is, we should throw a proper exception, because it's an invalid state for the menu, we shouldn't hide that.

It won't be null if it was was added by the UI. But if you add it from other sources like migration, it could be null. The code should avoid throw an unexpected a null reference exception.

@Piedone
Copy link
Member

Piedone commented Apr 15, 2024

Is it OK if it's null? I don't think a null Href makes sense. Why would we have such menu items?

@MikeAlhayek
Copy link
Member Author

Is it OK if it's null? I don't think a null Href makes sense. Why would we have such menu items?

It is not okay. But, we can incorrectly migrate a site with a null value outside the driver logic. Maybe line 12 should read this instead

Model.Href == null || Model.Href.ToString() == "#"

this way if there is an incorrect null value, we treat it as #

@Piedone
Copy link
Member

Piedone commented Apr 15, 2024

That would explain how it can happen at all, but the corresponding issue is about Href being null for a menu item coming from code (that I can't reproduce and am baffled about how is possible, to begin with).

Having

Model.Href == null || Model.Href.ToString() == "#"

looks good, but this wouldn't yet fix the problem in the linked issue, just hide it. We can then merge this PR but I'd wait for the issue author to confirm what the root of the problem is.

@MikeAlhayek
Copy link
Member Author

looks good, but this wouldn't yet fix the problem in the linked issue, just hide it. We can then merge this PR but I'd wait for the issue author to confirm what the root of the problem is.

The root of the problem is storing incorrect value in the database. But, it is better if out code does not throw unexpected exception. So we can treat null as '#' so it does not break the app, but the menu item will not be useful.

@Piedone
Copy link
Member

Piedone commented Apr 15, 2024

This isn't coming from the DB. Read from here: #15752 (comment)

@MikeAlhayek
Copy link
Member Author

@Piedone yes we should understand how that menu item is added. But, I also think we should also take this PR

@Piedone
Copy link
Member

Piedone commented Apr 16, 2024

That's what I also said :).

Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I'd like to give Italo more chance to reply under the issue. I'll merge this on Monday otherwise.

@Piedone Piedone merged commit 97fd760 into main Apr 22, 2024
6 checks passed
@Piedone Piedone deleted the ma/avoid-null-exception branch April 22, 2024 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Null reference exception in the Admin theme
2 participants