search

joshkosmala / silverstripe-tenon

Check the accessibility of your SilverStripe site with this module that integrates with Tenon
Other
16 stars 3 forks source link

Causes unnecessary server load by firing off AJAX request on every page load #6

Open chillu opened 9 years ago

chillu commented 9 years ago

Looking at tenon_post.js, its an unconditional $.post(). While not all of these cause an API request to Tenon, its still basically two hits to the server for every page visited. Particularly for heavily cached large sites in multi-server environments, that's not a great solution. It also assumes that the public web servers have access to dynamic services, which isn't always the case (e.g. for locked-down, statically published websites)

Both for performance and security reasons, I would recommend triggering the tenon POST in onAfterPublish by default. Since its an external API request which can take a few seconds, this should probably use the https://github.com/silverstripe-australia/silverstripe-queuedjobs module.

In case you want to retain the tenon_post.js approach, I would suggest abstracting a TenonService class from TenonAjax in order to make the logic more pluggable.

/cc @camfindlay

camfindlay commented 9 years ago

Agreed, this might make I nice set of features for a new version perhaps? @leighelse @joshkosmala it might be a good idea to tag this release of the module as an 0.1.0 (following semantic versioning). This makes it nice and easy for users to include in the project composer file and if you do another major version in the future it will help with the upgrade path. :+1:

jasonkiss commented 9 years ago

+1 for triggering Tenon POST with onAfterPublish. Would also integrate better with dev and author workflows, instead of requiring someone to visit the page after it's published.