MagicMirror icon indicating copy to clipboard operation
MagicMirror copied to clipboard

Fix crash possibility if `module: <name>` is not defined

Open bugsounet opened this issue 1 year ago • 7 comments

Fix #3442

bugsounet avatar May 10 '24 12:05 bugsounet

Just a thought: Wouldn't that be a thing that belongs in check_config.js?

KristjanESPERANTO avatar May 13 '24 09:05 KristjanESPERANTO

yes, also.

sdetweil avatar May 13 '24 11:05 sdetweil

check_config.js check only syntax but it does not check the contents of the file

should we make some other checks on it too ?

bugsounet avatar May 13 '24 16:05 bugsounet

well, this one is the first actual content that is important.. altho position should be checked too.. at least valid if specified.. that is another crash..

the user that reporteds the split problem said they ran config:check but it was good..

sdetweil avatar May 13 '24 17:05 sdetweil

So, we have to check module: <name>and position: <valid position> (if defined)

we check it on check_config.js and MM core ?

Personally, I think it's essential to apply the rules to both

bugsounet avatar May 13 '24 17:05 bugsounet

I think so.. never crash..

sdetweil avatar May 13 '24 17:05 sdetweil

ok, I will see to code it

bugsounet avatar May 13 '24 17:05 bugsounet

Right, I will try to add needed rules with eslint in check_config.js ;)

I have never done this kind of test. bugsounet, it's time to learn :)

bugsounet avatar May 17 '24 17:05 bugsounet

module name error: image

position error: image

No error: image

I'm not sure that i can integrate it into eslint

What do you think @sdetweil ?

bugsounet avatar May 18 '24 12:05 bugsounet

Before approuve some new deps. Thanks to review this :>

bugsounet avatar Jun 01 '24 15:06 bugsounet

If I use a module with a valid position and disabled: true , e.g.

		{
			module: "weather",
			header: "default weather module current",
			position: "top_left",
			disabled: true,
			config: {
				weatherProvider: "openmeteo",
				lat: 50.17659,
				lon: 8.62685,
				useCorsProxy: true,
				roundTemp: true,
				showPrecipitationProbability: true,
				showHumidity: "wind",
				showUVIndex: true,
				appendLocationNameToHeader: false,
				windUnits: "kmh",
				showFeelsLike: false,
				bridgeId: "abc",
			},
		},

I get a warning

[2024-06-07 23:21:00.091] [WARN]  No module name found for this configuration: {
  module: 'weather',
  header: 'default weather module current',
  position: 'top_left',
  disabled: true,
  config: {
    weatherProvider: 'openmeteo',
    lat: 50.17659,
    lon: 8.62685,
    useCorsProxy: true,
    roundTemp: true,
    showPrecipitationProbability: true,
    showHumidity: 'wind',
    showUVIndex: true,
    appendLocationNameToHeader: false,
    windUnits: 'kmh',
    showFeelsLike: false,
    bridgeId: 'abc'
  }
}

Is this intended? I think we need no such warning for disabled modules ...

khassel avatar Jun 07 '24 21:06 khassel

i agree, no warning there.

sdetweil avatar Jun 08 '24 02:06 sdetweil

sure, it's not logical

bugsounet avatar Jun 08 '24 11:06 bugsounet

fixed

bugsounet avatar Jun 08 '24 12:06 bugsounet

image

It's boring to redo the packages :/

bugsounet avatar Jun 11 '24 16:06 bugsounet

ping @rejas

bugsounet avatar Jun 20 '24 14:06 bugsounet

@rejas:

  • I see that postions is again used in main.js. So I use it with MM.getAvailableModulePositions --> I able to have all position in loader.js

  • I have move position utils in utils.js --> used in app.js with Utils.moduleHasValidPosition()

bugsounet avatar Jun 23 '24 12:06 bugsounet

@rejas: tell me

  • If it's ok for you or not ?
  • If you want really push it for MM² v2.28 ?

Sorry to say this but it's discouraging (this PR is open since May 10) :/

bugsounet avatar Jun 24 '24 17:06 bugsounet

@rejas: tell me

* If it's ok for you or not ?

* If you want really push it for MM² v2.28 ?

Sorry to say this but it's discouraging (this PR is open since May 10) :/

Of course its ok for me (I do trust your code :-) I was jsut pondering a lot if we could move ALL the position usages into the same file. but that would be as we say in germany: in schönheit sterben (meaning "to die in beauty" aka to work too much and never finish :-D

So sorry for my slow approbval, but will do now!

rejas avatar Jun 24 '24 19:06 rejas