Refactor render_social_share_buttons helper

[gravitystorm]

The implementation of render_social_share_buttons can be refactored in a number of ways:

• html_safe must not be used. It does not convert strings to a safe representation, it is an assertion that the string is always safe given every possible input. This method is probably too complex for us to assert this today, and certainly not to ensure that everything involved is perfectly safe when features are added or removed from the helper in future. See e.g. opengraph_tags helper for the use of safe_join to combine tags in loop.
• No need for a render_ prefix in helper methods. See e.g. opengraph_tags helper
• There are only two opts ever used in the method, :title and :url. Since there are only two, it would be better to call the method with these explicitly, ideally as keyword arguments so that the order doesn't matter.
• The filter_allowed_sites method behaves surprisingly. If you pass it some sites, it will only return a subset of those sites. However, if you pass an empty set, instead of an empty list you instead get a full list of sites that were not requested. However, it's probably unlikely to be important, because...
• The filter_allowed_sites method is unnecessary and unused code. It should be removed, since unnecessary and used code is both a source of bugs, and also makes it harder for new developers in future, who need to read and understand it. Personally I've spent time trying to review how it works and what it's trying to do, before realising that it's not used anywhere.

[AntonKhorev]

We have enough unused code in helpers. For example auth_button has options argument. Options for what? For link or image? Turns out it's for path. But it's never used. user_image_url also has options, also unused, and if you passed it, it would be used only in one branch. However all of that is insignificant compared to classic pagination #5205.

[kcne]

First of all, thank you for taking the time to do this post-merge review @gravitystorm . I wish these points had been raised earlier so I could have reassessed and addressed them before merging.

I completely agree with your observations. As mentioned earlier, I drew some inspiration from the rails library mentioned in the issue. The filter_allowed_sites method was intended to validate a given list of sites and render only the valid ones. If no list was provided, it would default to rendering all sites, assuming that social share buttons without any provider wouldn’t be practical. That said, I agree the implementation could have been cleaner and better aligned with our use case.

I noticed that @tomhughes has already opened #5417 to address this. I wish i was active at the time issue was raised to do it instead. Never the less, I appreciate the feedback and will keep this in mind as we refactor and finalize social sharing functionality.

[gravitystorm]

Fixed by #5417

We have enough unused code in helpers.

Yes, there's lots of other code to fix too, of course. We can work through each thing you mention individually.

I wish i was active at the time issue was raised to do it instead.

No worries, there's plenty more work for everyone!