Problem

When creating a rest resource that sends an sms using the module, the following exception appears when receiving a response from the resource:

A fatal error occurred: The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early. Returned object class: Drupal\\rest\\ResourceResponse.

Solution

Similarly to how #2630808: Response::getResponse() does not collect cacheability metadata from Url object solved it, the Url generated should collect bubbleable metadata in SmsMessageProcessor::deliveryReportUrl().

CommentFileSizeAuthor
#5 TestSms.php_.txt880 bytesnyariv
#5 rest.settings.yml129 bytesnyariv
#2 2790673-url_bubbleable_metadata.patch792 bytesnyariv
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nyariv created an issue. See original summary.

nyariv’s picture

nyariv CreditAttribution: nyariv commented
FileSize
2790673-url_bubbleable_metadata.patch792 bytes
dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi as a volunteer commented

Can you please post steps to reproduce? Linking to your own code if you desire, and providing instructions.

Can you supply tests with your patch.

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi as a volunteer commented

I also don't exactly understand why the SMS internals want to have anything to do with the caching system?

nyariv’s picture

nyariv CreditAttribution: nyariv commented
FileSize
rest.settings.yml129 bytes
TestSms.php_.txt880 bytes

1) Copy attached resource class to [your_module]/src/Plugin/rest/resource. You will need to remove .txt from file name and change the namespace of the class to your module.
2) Enable core's rest module and import attached config
3) Enable permission 'Access GET on Test sms resource resource' for anonymous user
4) Do a GET request to /test-sms/111?_format=json it should say 'Sms sent' but instead it shows the exception error.

I do not understand fully the source of the issue, but it has to do with Url caching.

nyariv’s picture

nyariv CreditAttribution: nyariv commented

A workaround I found was to use
$response = new Response('Sms sent', 200);
instead of
$response = new ResourceResponse('Sms sent', 200);
which is a cacheable response. This only means my resource will not be cached.

nyariv’s picture

nyariv CreditAttribution: nyariv commented

This is for a module I am maintaining that sends verification codes through sms and heavily relies on sms framework. One of the features is to support verification through rest/services. I have committed the code with the above workaround, which is OK since I do not need caching anyways.

Would love your opinion,
https://www.drupal.org/project/mobile_number

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi as a volunteer commented

Do a GET request to /test-sms/111?_format=json

Im not sure if this is the same code in your module, its arguable whether you should be using GET for your resource?

which is a cacheable response. This only means my resource will not be cached.

I dont think you want to cache a send operation...?

Would love your opinion, https://www.drupal.org/project/mobile_number

I'll add it to my todo list :). What is the difference between the verification code system we are shipping with sms_user 8.x, and mobile_number?

almaudoh’s picture

almaudoh CreditAttribution: almaudoh commented

I was able to replicate the behavior following #5. However, I'm not able to understand how url->setAbsolute()->toString() without capturing cacheability metadata ends up in a render call outside a render context.

almaudoh’s picture

almaudoh CreditAttribution: almaudoh commented

What is the difference between the verification code system we are shipping with sms_user 8.x, and mobile_number?

I'm also seeing an area where we can consolidate the functionalities of the two modules here.

nyariv’s picture

nyariv CreditAttribution: nyariv commented

Indeed, a user of my module did report confusion about seeing an extra mobile number field in the user account page.

mobile_number is its own field type, and acts as a pivot to any integrations needed for a mobile number data type. So verification needs to be possible on any entity type that the field is added to, including also for use with two factor authentication. It would make sense to couple verification handling with the mobile number field and simplify smsframework to focus on simply being a provider for sending sms of any kind.

Because of TFA, verification codes need to receive a password-like treatment and expiration when storing them in the database. Perhaps verification codes need to be handled by a third module, but I am not aware of such a module so I created my own implementation.

I dont think you want to cache a send operation

When the send operation returns a verification token, which my module does, then caching is not desired.

Im not sure if this is the same code in your module, its arguable whether you should be using GET for your resource

I do use GET, perhaps you are right, POST would be more appropriate. I am having difficulties finding documentation for implementing a POST resource for rest though at the moment.

I'm also seeing an area where we can consolidate the functionalities of the two modules here.

I would be more than happy to collaborate here and see how reuse can be done between the two modules.

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi as a volunteer commented

I would be more than happy to collaborate here and see how reuse can be done between the two modules.

If we are just discussing fields for a moment, then you would just need to integrate with Drupals' phone number module.

Dissecting mobile_number field:

    $properties['value'] = DataDefinition::create('string')
      ->setLabel(t('E.165 Number'))
      ->addConstraint('Length', array('max' => 19));

    $properties['country'] = DataDefinition::create('string')
      ->setLabel(t('Country Code'))
      ->addConstraint('Length', array('max' => 3));

    $properties['local_number'] = DataDefinition::create('string')
      ->setLabel(t('National Number'))
      ->addConstraint('Length', array('max' => 15));

    $properties['verified'] = DataDefinition::create('boolean')
      ->setLabel(t('Verified Status'));

    $properties['tfa'] = DataDefinition::create('boolean')
      ->setLabel(t('TFA Option'));
  • value: the phone number itself.
  • country: this can use a service, or there can be a cache layer elsewhere. (libphonenumber)
  • local_number: duplicated data / cache / (libphonenumber)
  • verified: SMS Framework stores verification state separately to the entity. See \Drupal\sms\Entity\PhoneNumberVerification.
  • tfa: ...

Obviously this is just my opinion, and I'm not familiar with the internals of the project.

nyariv’s picture

nyariv CreditAttribution: nyariv commented

I tried integrating with phone module but it has it's own validation system which conflicts with mine so I was not able to do that. I explained here why mobile numbers should have it's own field type, plus the phone module is not actively maintained and they say on the project page to use 'telephone' module instead, which I can and have integrated with.

About the field properties:

  • value - needs to stay
  • country - you cannot extract country from value easily which indeed requires a service, but that would not work well with views when you want to filter by country.
  • local_number - same with local number, numbers can have leading zeros which are not expressed in 'value' and require knowing the country, so it also requires processing. I make sure value/local_number/country match on presave.
  • verified - I believe the verification status should be saved along with the number, which is possible with a custom field type. I have opened a separate issue to discuss this, please say what think there, pros/cons etc. Feel free to edit the summary, I have added points I know about my own module, I am sure there are other differences you could find. After writing it though I figured the two systems could coexist, SmsF for telephone, MN for itself.
  • tfa - whether tfa is enabled for this number, used only on user entity, otherwise always 0.

If you have the time, install MB just to check it out, I am sure you could get some ideas for your own module. There are some limitations which prevented me from extending telephone like you did, like adding my own field constraints to telephone, or allowing users to select extensions to existing field types beyond just widget selection. I prefer to work with Drupal's intended api and avoid hackish ways to achieve these.

bradjones1’s picture

bradjones1
Primary language English
CreditAttribution: bradjones1 as a volunteer commented
Related issues: +#2638686: Exception in EarlyRenderingControllerWrapperSubscriber is a DX nightmare, remove it

The core of this issue report is basically #2638686: Exception in EarlyRenderingControllerWrapperSubscriber is a DX nightmare, remove it. The Url::toString() sets certain cacheable metadata, which "leaks" because it's not included in the cache metadata of the response you're ultimately building with your REST service. If the cacheable metadata you have on that response includes the metadata otherwise "leaked," you could be good to go, but it's not exactly predictable (e.g., it could depend on the logged-in user, etc... the cacheable properties of the link you're generating.)

I might have bungled the explanation a bit, but I wanted to relate this since it is pretty much the issue you're having, not necessarily specific to Sms module.