On the get method from MonitoringSensorResultResource.php there is a $url variable that uses the route name hardcoded. My suggestion is to use the current route name (\Drupal::routeMatch()->getRouteName() but using DI).

The reason behind this would be to be able to have my own RestResource extending from it and being able to define a different uri_path and authentication methods. To be more specific, I do not want to rely on the basic_auth module, as things stand, I must install the basic_auth module for that sensor to work. That module brings issues with environments using basic auth at the webserver level (Nginx).

CommentFileSizeAuthor
#11 interdiff.3154706.7-11.txt5.56 KBemersonreis.dev
#11 3154706-11.monitoring.Use-current-route-name-instead-of-hardcoded-route-name.patch2.38 KBemersonreis.dev
#7 interdiff.3154706.5-7.txt1.99 KBemersonreis.dev
#7 3154706-7.monitoring.Use-current-route-name-instead-of-hardcoded-route-name.patch5.33 KBemersonreis.dev
#5 3154706-5.monitoring.Use-current-route-name-instead-of-hardcoded-route-name.patch3.34 KBemersonreis.dev

Comments

dungahk created an issue. See original summary.

emersonreis.dev’s picture

emersonreis.dev
he/him/his
Primary language Portuguese, Brazil
Location Balneário Camboriú
commented
Status: Active » Needs review
emersonreis.dev’s picture

emersonreis.dev
he/him/his
Primary language Portuguese, Brazil
Location Balneário Camboriú
commented

I'm happy to work on a patch for this. If I get time I will look at doing it tomorrow, but it would be good to have an input from the maintainers.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Open to improving things. Not aware that we have a hardcoded dependency on basic auth, I would assume that's just the optional configuration? Not quite sure how to avoid hardcoding that though, especially on the list level.

emersonreis.dev’s picture

emersonreis.dev
he/him/his
Primary language Portuguese, Brazil
Location Balneário Camboriú
commented
StatusFileSize
new 3154706-5.monitoring.Use-current-route-name-instead-of-hardcoded-route-name.patch3.34 KB

Here is the patch.

Basically, what this does is it allows developers to have their own RestResource extending MonitoringSensorResultResource without depending on basic_auth. This allows developers to use different authentication methods and not having basic_auth enabled. This is my use case.

At the moment, I need to enable basic_auth because otherwise, the optional config will not be enabled and the route 'rest.monitoring-sensor-result.GET' will not exist, thus throwing an exception. But I don't need that route nor basic_auth, I have my own RestResource with my own authentication method.

This does not remove the basic_auth dependency as that would probably be a discussion for another issue!

Status: Needs review » Needs work

The last submitted patch, 5: 3154706-5.monitoring.Use-current-route-name-instead-of-hardcoded-route-name.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

emersonreis.dev’s picture

emersonreis.dev
he/him/his
Primary language Portuguese, Brazil
Location Balneário Camboriú
commented
StatusFileSize
new 3154706-7.monitoring.Use-current-route-name-instead-of-hardcoded-route-name.patch5.33 KB
new interdiff.3154706.5-7.txt1.99 KB

Fixing tests

emersonreis.dev’s picture

emersonreis.dev
he/him/his
Primary language Portuguese, Brazil
Location Balneário Camboriú
commented
Status: Needs work » Needs review
emersonreis.dev’s picture

emersonreis.dev
he/him/his
Primary language Portuguese, Brazil
Location Balneário Camboriú
commented
Assigned: Unassigned » emersonreis.dev
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
+++ b/tests/src/Functional/MonitoringServicesTest.php
@@ -162,10 +162,13 @@ class MonitoringServicesTest extends MonitoringTestBase {
+   * @param bool $is_list_route
+   *   If the route that the result came from was the list or not.
    */
-  protected function assertSensorResult($response_result, SensorConfig $sensor_config) {
+  protected function assertSensorResult($response_result, SensorConfig $sensor_config, bool $is_list_route = FALSE) {
     $this->assertEqual($response_result['sensor_name'], $sensor_config->id());
-    $this->assertEqual($response_result['uri'], Url::fromRoute('rest.monitoring-sensor-result.GET' , ['id' => $sensor_config->id(), '_format' => 'json'])->setAbsolute()->toString());
+    $route_name = $is_list_route ? 'rest.monitoring-sensor-result.list.json' : 'rest.monitoring-sensor-result.GET';
+    $this->assertEqual($response_result['uri'], Url::fromRoute($route_name , ['id' => $sensor_config->id(), '_format' => 'json'])->setAbsolute()->toString());

but that's exactly my point from above. now you point the list route. but that's not the idea, that doesn't make sense. the list route doesn't have an id argument, it's the list.

Agree with this a problem, always been aware of that, but your solution doesn't work.

Still don't quite get the basic auth argument though. The problem is hardcoding the config entity name, we don't care what authentication method is in there As a workaround, you could just name your own rest config the same :)

emersonreis.dev’s picture

emersonreis.dev
he/him/his
Primary language Portuguese, Brazil
Location Balneário Camboriú
commented
StatusFileSize
new 3154706-11.monitoring.Use-current-route-name-instead-of-hardcoded-route-name.patch2.38 KB
new interdiff.3154706.7-11.txt5.56 KB

I see. I totally misunderstood the purpose of the URI on the response! My apologies.

I am attaching a new patch, let me know if this makes sense.

emersonreis.dev’s picture

emersonreis.dev
he/him/his
Primary language Portuguese, Brazil
Location Balneário Camboriú
commented

An example of how a custom RestResource would look like

class MyRestResource extends MonitoringSensorResultResource {

  /**
   * {@inheritdoc}
   */
  public function __construct(
    array $configuration,
    $plugin_id, array
    $plugin_definition,
    array $serializer_formats,
    SensorManager $sensor_manager,
    SensorRunner $sensor_runner,
    LoggerInterface $logger,
    RendererInterface $renderer
  ) {
    parent::__construct($configuration, $plugin_id, $plugin_definition, $serializer_formats, $sensor_manager, $sensor_runner, $logger, $renderer);
    $this->route = 'rest.my-monitoring-sensor-result.GET';
  }

}
emersonreis.dev’s picture

emersonreis.dev
he/him/his
Primary language Portuguese, Brazil
Location Balneário Camboriú
commented
Status: Needs work » Needs review