JeroenDeDauw / ParamProcessor

Declarative parameter processing library
https://entropywins.wtf
Other
20 stars 4 forks source link

ParserOptions by params definition #10

Closed osnard closed 8 years ago

osnard commented 8 years ago

Hi!

Is there any way to pass options to the parser object using the param definition? As far as I can see the options from param definition are only passed to the Validator object. Is that right or am I missing something?

Greetings, Robert

JeroenDeDauw commented 8 years ago

I cannot answer your question as I'm not sure what you mean by "the parser object". Is this a MediaWiki parser object? Do you mean the Processor class? Same goes for "the Validator object". Is this an instance of the Validator class from the Validator MediaWiki extension?

osnard commented 8 years ago

Sorry, you are right. My quesrtion is a little too unspecific.

No, I do not mean the MediaWiki Parserobject. I am talking about classes that implement ValueParsers\ValueParser interface (I believe it is from data-values), which are used by ParamProcessor to parse the value before passing it to an instance of ValueValidators\ValueValidatorObject.

JeroenDeDauw commented 8 years ago

Sorry for taking such a long time to get back to you.

The ValueParser objects do indeed not accept any options. Perhaps they should - there are quite a few design problems in this code. What is your actual use case for this? Generally the parsers can parse all values for a given type of parameter. For instance, if you want to have a coordinate parameter where it can be specified what format it should be in, then you'd have a parser that can parse all formats, and a validator that takes the specified format and makes sure it is that format.

osnard commented 8 years ago

Well, actually I try to implement a "MediaWikiNamespaceList" Validator/Parser. It should take something like

"2,File,template,-1"

and provides my tag extension with

array( 2, 6, 10, -1 )

I know that the param definition has a 'delimiter' setting that splits a value before passing it to the Parser. The return value of the Parser is then passed to the Validator. But If there is an invalid item in the list, e.g. -13, then the Parser can only return a null or false as it is responsible to resolve the item value into a namespace ID. The Validator retrieves this and can tell that it is invalid. But it does not know where the invalid value came from and thus can not provide a proper error message to the user (e.g. "Value -13 is not a valid namespace"). So my plan was to do the splitting of the list within the Parser. The Parser then provides something like

array(
    "2" => 2,
    "File" => 6,
    "template" => 10,
    "-1" => -1
    "-13" => false
)

to the Validator. The Validator can then do it's additional checks (i.e. if values are within a configurable list of allowed namespaces) and give feedback to the user.

But if I put the splitting logic for the list into the Parser (instead of using the ParamDefinition 'delimiter') I need a way to configure the 'delimiter' that the Parser uses.

Hope that makes my case clearer. Btw. thanks for your hints so far.

JeroenDeDauw commented 8 years ago

Why do you need to configure this? And on what level? Just once for the "MediaWikiNamespaceList" parameter type, or per parameter of this type? The later you cannot do without making some changes to ParamProcessor. The former looks like you can't either, at least not without using global state, as the parameter definitions (example)

    $GLOBALS['wgParamDefinitions']['coordinate'] = array(
        'string-parser' => 'DataValues\Geo\Parsers\GeoCoordinateParser',
    );

do not allow for dependency injection. That is a serious design flaw... (you're certainly invited to fix that if you feel like it.)

Why do you want this relatively simple task to be done via ParamProcessor? You could just use the list parsing stuff, and then do a pass on this in the code you control.

osnard commented 8 years ago

My goal is to create a generic tag extension mechanism for the BlueSpice project [1][2]. There are a bunch of tag extensions that all do their own validation of input parameters, which is annoying. I want to use ParamProcessor to parse and validate all input data before even passing it to the tag extension callbacks.

As many of the tag extensions take a list of namespaces/titles/categories/usergroups as parameter, I wanted to implement proper Parser and Validator classes and register them with wgParamDefinitions [3]. That way I could simply use them within the various tag extensions [4] and have a unified processing and error handling/output mechanism for all of them. The callback code then can just work with the params and does not need to worry about the validity of the params that are passed to it.

Unfortunately, as this is work on legacy code, the lists may have different delimiters. E.g. One extension wants a namespace list in form of "2,File,template,-1", and the next one wants "2|File|template|-1". Therefore I need a way to configure the delimiter (at least; there may be other cases) for each extension.

Sorry for spamming you with so much context, but I think it might help here.

[1] A MediaWiki distribution for business intranets; https://www.mediawiki.org/wiki/BlueSpice [2] https://phabricator.wikimedia.org/tag/bluespice/ [3] https://gerrit.wikimedia.org/r/#/c/278280/ [4] https://gerrit.wikimedia.org/r/#/c/278282/

JeroenDeDauw commented 8 years ago

I think I understand the general gist of what you are trying to do. However I'd still be inclined to try avoid using ParamProcessor for the part you are having problems with, and do code reuse and abstraction via a different mechanism, given the sorry state of the ParamProcessor code. Alternatively I'd tackle the ParamProcessor design issues causing problems here, such as the lack of dependency injection. (I'm willing to provide some support if you decide to do that, though cannot take on the task myself.)

Are you familiar with the ParserHooks library? Sounds relevant for what you are doing, and is build on top of ParamProcessor, which you are already using. It does not solve the problem you have here, though might save other work, and is itself actually reasonably designed and well tested.

osnard commented 8 years ago

I think I've already found a suitable solution for my problem in the meantime. I went away from the idea of implementing a "namespacelist" type but rather implemented a "namespace" type and using "isList" of the ParamDefinition. Error reporting to the user is not as smooth as I hoped it would be, but it is sufficient. Maybe I can contribute a few changes to ParamProcessor to improve this.

I know ParserHooks extension and really like it. But I decided to implement a own solution because I also wanted to have some generic things to happen with each and every tag we have in BlueSpice (e.g. adding classes and data attributes to the container element). I plan to use ParamProcessor for other input processing as well. E.g. in API modules.

One of the main reasons to choose ParamProcessor library was that it is used by SemanticMediaWiki extension.

osnard commented 8 years ago

By the way I am going to close this issue. Thanks for your support so far. I might have other questions as I make progress with the implementation.