Skip to content

Conversation

@StrixG
Copy link
Contributor

@StrixG StrixG commented Feb 21, 2020

Closes #1253

@StrixG StrixG requested a review from qaisjp February 21, 2020 11:06
@ccw808
Copy link
Member

ccw808 commented Feb 21, 2020

'protected' seems slightly ambiguous. Should we make the name more indicative of the base functionality? e.g. isResourceStoppable or something better

@StrixG
Copy link
Contributor Author

StrixG commented Feb 21, 2020

'protected' seems slightly ambiguous. Should we make the name more indicative of the base functionality? e.g. isResourceStoppable or something better

I think isResourceStoppable would be more ambiguous than isResourceProtected. Because protected resource is stoppable, but only with stopResource, restartResource functions.

@Dutchman101
Copy link
Member

I don't think this is the right solution at all, the runcode resource (and admin panel code executor) would first need to check if the functions stopResource or restartResource are entered, and then perform the isResourceProtected check.. as admins can otherwise just run these functions (in a resource that can run code) without the checks.
We can do it for default resources (runcode and admin panel) but otherwise that's a hassle, not all scripters are that intuitive (if we can do better by default, then why not..) and you could theoretically even implement runcode functionality through a runcode query, with the 'new' interface lacking these checks.

I believe that the mtaserver.conf value 'protected' should rather be authoritative.. just return false from any functions capable of manipulating the running state (stopResource and restartResource) and cancel it. Server owners setting this, from its naming since years, generally assume(d) it's safe without further action needed in their scripts.. if we just correct it (make it authoritative like it's supposed to be), an server update will solve it all at once, instead of the owner having to take notice and update scripts manually. That's another benefit of doing it how I consider it would be properly done..

@qaisjp
Copy link
Contributor

qaisjp commented Feb 22, 2020

Hm, protection can indeed be slightly confusing but I can't think of something other than stoppable, which is also confusing to me.

Adding this function is fine because scripts should be able to get this attribute. Alternatively we could extend https://wiki.multitheftauto.com/wiki/GetResourceInfo to support ".protected" as a key instead of adding another function?

See NameStartChar — I think . is an invalid start character, so it could never appear in the meta XML file.

I'm not sure about whether scripts should be able to start or stop protected resources. But I generally find the existence of resource "protection" a little weird and it would be useful to get more info about why (what scenarios) people protect resources. Discussion about changing the behaviour is a separate issue though, since we'd still need isResourceProtected.

@patrikjuvonen patrikjuvonen added the enhancement New feature or request label Feb 22, 2020
@qaisjp
Copy link
Contributor

qaisjp commented Mar 30, 2020

My suggestion above about using getResourceInfo is kind of ridiculous - in the end there's no big deal in adding an extra function. Especially considering not that many resource-related functions have been added in the last ten years.

I'm going to merge this now - if we come up with a better name, we can change it

@qaisjp qaisjp merged commit 0af8546 into multitheftauto:master Mar 30, 2020
@qaisjp qaisjp added this to the 1.6 milestone Mar 30, 2020
@StrixG StrixG deleted the isresourceprotected branch March 30, 2020 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add isResourceProtected

5 participants