-
-
Notifications
You must be signed in to change notification settings - Fork 527
fix: undefined property access in case of deleted macro lookup #1685
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
Conversation
Signed-off-by: Mathis Mensing <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @matmen, I haven't been able to reproduce this, but codewise I see no problem with this change!
Oddly, I can't even find any macro2.name on the code...
From what I can see, we have macro settings in the store that have no name, is that it?
That should not be the case, but this will fix the issue, no question about it!
|
We do have similar code on the macro/mutations.ts, I do wonder if the same change should be done there? |
yep, me neither, i think that's just what the compilation changes it to tho. If i jump to the source code from the dev tools it marks that line, and I've tested the fix to be working ;)
yep, i guess it's not being populated if it's not in the config. Alternatively we could just remove it from the saved macros, but that would delete all the saved settings if a user just decides to comment out the macro from the config for testing
I don't think so - the code in there never runs because macros that aren't in the config can't be mutated either (they don't show up in the UI) |
|
The thing is that the stored settings are populated from whatever is currently on the existing macros... So I don't get how one could go without a name! I get that this fixes the problem, I just don't see how deleting/commenting out a macro would cause this as we already have the macro setting stored - we just won't use it as he macro is gone now! |
well, that's the thing, we do use the stored macro settings as we iterate over each one in I can take a look at what's actually stored in my moonraker db tomorrow - perhaps the stored data was corrupted somehow and is just missing the |
|
Agreed, merge away, I see no problem with this change! |
When a macro is removed from the config, it isn't deleted from the stored config in the database. This can lead to an undefined property access while looking up the stored macro data: