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

All in One Accessibility Plugin #164

Closed
wants to merge 1 commit into from

Conversation

skynetindia
Copy link

@skynetindia skynetindia commented May 15, 2024

We have create All in One Accessibility plugin. Please check and let us know if anything else required

@garvinhicking
Copy link
Member

Hi!

Thanks for your contribution! However this plugin to me looks more like it should be offered by you, because it also relies on your webservice as well as payment information. Currently our plugin repository is more suitable for "open source" services or very common widgets.

Some code comments:

  • It seems you've used some other plugins as a basis, which is a good thing, but you might want to remove references/comments to those plugins (i.e. changelog, translation files, header of the plugin itself)
  • Since you already provide translation files, you should use those instead of hardcoding the english strings into the plugin
  • The introspect_config_item() method is called for every configuration item and in your case makes a CURL request to a URL. That's ok, but if in the future you add more config items, that might be a problem - better is to only execute curl specifically for the "configuration" key "allinoneaccessibility" - for example with a if/switch/case statement like you can find in other plugins.
  • You have the version added twice as a property bag parameter, one is redundant :)
  • The propbag attributes support_url and support_email are not evaluated by Serendipity
  • CURLOPT_POSTFIELDS uses a "example4644.com" domain, I don't think that's right and would need to be the actual hostname of the instance?
  • Using "echo" for the introspection is a problem. You need to utilize the proper API for emitting a custom configuration field that contains your javascript. Check out https://github.com/s9y/Serendipity/blob/99ce856b9ea37ba214ce4c7c96414a1e321d80e6/include/functions_plugins_admin.inc.php#L327 to see which possible "type" values a introspect_config_item() method call can set. "content" or "custom" would be the proper ones for you.
  • Embedding iframes to manage content of the widget within the s9y backend does not feel good to me and could introduce security issues. I'd rather suggest to add a link there like "manage my subscription" and then your site can be called.
  • The same with embedding an iframe directly in case of unconfigured domain is a red flag to me

The vital part of the plugins seems to be generate_content() that calls a javascritp widget from your site. You may thus not even need a custom Serendipity plugin with this, you could just allow Serendipity blog owners to create a "HTML Nugget" on their site, and then they can copy+paste your javascript/iframe link in there to place it. You should mention though that embedding a service like this under GDPR/DSGVO rules at least in germany would require visitor's consent before submitting data to you; so such a plugin would first need to ask for consent before automatically including and (possibly security-relevant) sending such data to your servers.

Hope my feedback can be of value to you!

Regards,
Garvin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants