51Degrees / device-detection-php-onpremise

On-premise implementation of the 51Degrees Device Detection engines for the Pipeline API
Other
1 stars 3 forks source link

Getting started example .json configuration setting ignored #11

Closed jwrosewell closed 1 year ago

jwrosewell commented 1 year ago

This issue relates to the examples/onpremise/gettingStartedWeb example.

The .json configuration file contains a property called endpoint with the value /json as part of the build parameters for the JavascriptBuilderElement engine.

                "BuilderName": "fiftyone\\pipeline\\core\\JavascriptBuilderElement",
                "BuildParameters": {
                    "endpoint": "/json",
                    "minify": true,
                    "enableCookies": true
                }

When this setting is retrieved in the JavaScriptBuilderElement constructor the value is not present and the default empty string is used.

Because this value has to match the end point of the JsonBuilder builder in the example, the example doesn't work.

We need to determine why the setting from the .json configuration is being ignored.

A work around is to modify line 46 of the JavaScriptBuilderElement to use the same end point as the example.

justadreamer commented 1 year ago

Looking at the code the endpoint seems to be read fine from settings and stored in the $this->$settings["_endpoint"], but then it is later never used. Specifically unlike host and port it is never assigned to $vars["_endpoint"], which evaluates to falsy here. The fix should be simply to add line prior to that if:

$vars["_endpoint"] = $this->$settings["_endpoint"]

Will verify this a bit later.

justadreamer commented 1 year ago

actually scratch that, I see the assignment above

kirstin51D commented 1 year ago

This issue is being addressed here: https://github.com/51Degrees/pipeline-php-core/pull/10

justadreamer commented 1 year ago

The root cause described in the PR above is that the JavascriptBuilderElement is actually first created in the buildFromConfiguration here, but then the default JavascriptBuilderElement is added in the PipelineBuilder->build() function here - and this is the one that is used apparently.

The fix then attempts to deduplicate the elements giving precedence to the ones created from configuration, but also it then tries to ensure that the Sequence element is the first element in the pipeline as the spec requires.

Maybe it is an overcomplication to ensure that Sequence is the first one in this fix - it would be enough to just deduplicate the elements and make sure that the ones that are coming from configuration are present - and then it is on the whoever created the configuration to ensure the Sequence element and other are at the correct positions? This would greatly simplify the fix and remove some dark magic of searching for the Sequence element and putting it in the correct position (it could have been done differently using the explicit class name or dataKey, but in the PR there are some tricks with array_shifts).

justadreamer commented 1 year ago

This has been addressed in the latest pipeline-php-core dependency and device-detection-php-onpremise. The example now respects the setting specified in the configuration.