Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default parameter values and caching #9

Open
Jako opened this issue Dec 8, 2016 · 4 comments

Comments

@Jako
Copy link

commented Dec 8, 2016

The snippet seems to have issues with default parameter values and caching. I have to use explicit

&context=`[[*context_key]]` &published=`1`

Otherwise some cached values from another context are used.

@sepiariver

This comment has been minimized.

Copy link
Member

commented Feb 9, 2018

@Jako working on this for next release. Appreciate your input on these options:

  1. Add context_key to cacheKey filename to prevent multi-context collisions
  2. Use all scriptProperties variables, whether default or set, in the cacheKey hash.
    Does option 2 seem like overkill? It's significantly more code than option 1...
@Jako

This comment has been minimized.

Copy link
Author

commented Feb 9, 2018

Phew, 1. would be enough in most cases. 2. is better, since two snippet calls with some not default parameter values could be cached in the same cacheKey otherwise.

@Jako

This comment has been minimized.

Copy link
Author

commented Jul 1, 2019

It looks like this issue is still valid: https://community.modx.com/t/sitemap-for-multi-domain/1124

sepiariver added a commit that referenced this issue Jul 1, 2019
@sepiariver

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Ok so there's this: 6a2cec7

Only thing is, it doesn't include the default parameters other than context. There's a balance here:

  1. Include all default parameters, but then can only identify cache key after running all the parameter-setting logic, so there's a potential performance degradation for cache hits.
  2. Leave it as it is in that commit, and other default parameters may cause cache collision.

What do you think? I could go either way. 1st option is more work, clearly. Maybe we iterate towards that? But I'm wondering if it's worth the performance issue. I haven't quantified the effect.
@Jako

sepiariver added a commit to sepiariver/GoogleSiteMap that referenced this issue Oct 21, 2019
sepiariver added a commit that referenced this issue Oct 21, 2019
Issue #9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.