Issue fork wsdata-3581708

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

svitalis created an issue. See original summary.

svitalis opened merge request !10

vishal.kadam’s picture

vishal.kadam
Location Mumbai
commented
Issue tags: -wsdata_field.module
spotzero’s picture

spotzero commented
Status: Active » Needs work

Hey Shawn

I see what you're doing, but I don't think this is the correct place. The field doesn't actually know whether the wscall is cacheable or not. The wsconnector used might work in some way there the results aren't cachable in a predictable way.

That's why caching is managed here, just a little bit further down the chain:
https://git.drupalcode.org/project/wsdata/-/blob/3.0.x/src/Entity/WSCall...

You'll notice an important condition about that here:
https://git.drupalcode.org/project/wsdata/-/blob/3.0.x/src/Entity/WSCall...

However, what you've done is good. More aggressive default caching for repeated calls makes sense and using drupal_static makes sense. It just needs to be implemented deeper in the code. That also has the side benefit of making caching better for the whole module.

svitalis’s picture

svitalis commented

Thanks for the guidance Dave. Please let me know about the updated changes.

For wsdata_field.module, I left in an empty token check to prevent invalid calls.

spotzero’s picture

spotzero commented
Status: Needs work » Needs review
svitalis’s picture

svitalis commented
Issue summary: View changes

Hey Dave. Have you (or your team) had a chance to review this yet?

classiccut’s picture

classiccut commented

@svitalis I just took a quick look at the proposed code changes. The static caching part looks good. I'm just confused about the token replacement code in the wsdata_field module entity_load hook. Doesn't seem related to the caching, so maybe that's part of some other fix? Sorry, I'm just jumping on here for a review so I don't have the full picture. Thanks.

svitalis’s picture

svitalis commented

@classiccut, you're right that they're unrelated. The token replacement code is a separate fix that was included in this same MR. Its purpose is to prevent malformed API requests when a node has an empty field_booking_target field.

Without this, the token [node:field_booking_target] evaluates to an empty string, which results in a malformed path like /api/article/ being sent to the backend API (causing the 500 Internal Server Error we were investigating).

The fix evaluates all token replacements upfront, and if any resolve to an empty value, it silently skips the web service call entirely and sets the field to NULL rather than firing a bad request.

I'm happy to split it out into a separate MR if you'd prefer them on their own.