Code Review

[NickGreen]

Synopsis

This issue is a placeholder for a code review request. There are a couple of places that would be very helpful to focus:

1. Compatibility with the widest range of websites.
2. Best practices. Let's do things properly, and the "WordPress way"

Details

• This is a public plugin which we hope will be in widespread use.
• The biggest problems that we've found so far in testing are CSS collisions.
• We would like to keep it compatible back to PHP 7.4 for now, since there are still a good many sites on 7.4

[ahegyes]

Some of these things are pedantic but you asked for widest range and The WordPress way 😄

1. Add an empty index.php file in the root folder and inside the includes folder
2. Can this be a security risk for any reason? Doesn't feel like info we should be giving away in the plugin ... define( 'FEEDLAND_DEFAULT_USERNAME', 'davewiner' );
3. wp_register_script has recently changed the last argument from a bool to an array; it might be more forward-compatible and have better intent to replace false with array( 'in_footer' => false ) (see notes about WP6.3)
4. I'm not a fan of enqueuing FontAwesome mandatorily; maybe at least add an option to dequeue it in case it's already added by the theme or something else? Even better would be if we integrated with the official WordPress plugin which can then handle and resolve potential conflicts: https://wordpress.org/plugins/font-awesome/ (instructions on how to bundle it or require it or at least integrate with it here: https://github.com/FortAwesome/wordpress-fontawesome)
5. Similarly, I'm not a fan of forcing Google Fonts ... can't we add them using the 6.5 Font Library: https://make.wordpress.org/core/2024/03/14/new-feature-font-library/ ? (47% of WP sites are already running 6.5: https://wordpress.org/about/stats/); it's also a GDPR concern to just load fonts from GoogleAPIs.com 😅
6. Ditto on forcing Bootstrap to load ... this is becoming a very heavy plugin assets-wise for a simple blogroll 😅 Could we instead build a custom version of Bootstrap that includes only the parts we need? We can use the meta.load-css mixin to prepend all of it with an id/class/something and thus prevent any CSS conflicts with the rest of the theme;
7. Why not just exclude the phpcs check for Universal.Operators.DisallowShortTernary.Found? I always do that...
8. The self-update.php file doesn't have a check for ABSPATH ... like defined( 'ABSPATH' ) || exit; at the top.
9. You don't need to set user-agent in the wp_remote_get options ... WP will add one that includes the WP version and the blog name which is much more descriptive;
10. Line 35 in self-update.php returns void but the function is marked as returning array|false
11. There is no POT file in the repo
12. Instead of define should we do a defined || define check to allow overwriting constants in wp-config or something? For example, I can see the FEEDLAND_DEFAULT_SERVER constant as something we would want to overwrite to test against a staging server, e.g.
13. The bootstrap file does not checks that the "correct" version of PHP and WP are running; technically, the headers should prevent it from being activated on lower versions but there is nothing stopping anyone from changing the PHP version or downgrading the WP version on a site that has the plugin already active; see how the Team51 Plugin Scaffold does it: https://github.com/a8cteam51/team51-plugin-scaffold/blob/trunk/team51-plugin-scaffold.php
14. I'm not a fan of the fact that the JS variable is called BLOGROLL_OPTIONS ... first of all, it's not prefixed, and second, why is it screaming at me? 😅
15. Even in PHP7.4, we can still type-hint arguments and return values ... we're not doing that here 😄
16. We've recently talked about adding declare( strict_types = 1 ); to all our new projects ... it might be a good idea to do that here too; pewaj1-13Z-p2
17. Instead of enqueuing the styles/scripts inside the shortcode callback, what if we enqueued them on the action wp_enqueue_scripts with the conditionals has_shortcode()? Or would that not work because that only checks the post content and no the sidebar?

That's it for now, I'll add more if I see anything else 😄

[ahegyes]

Since the ID of all blogrolls is idBlogrollContainer this can lead to HTML validation errors if, for some reason, the blogroll is outputted more than once per page. Maybe the user wants to output it for multiple accounts?

What if we appended the value of wp_unique_id() to it?

[NickGreen]

Working checklist

• [ ] Add an empty index.php file in the root folder and inside the includes folder
• [ ] wp_register_script replace false with array( 'in_footer' => false )
• [ ] Use FontAwesome better
• [ ] Add fonts using the 6.5 Font Library instead of from Google
• [ ] Bootstrap considerations
• [ ] exclude the phpcs check for Universal.Operators.DisallowShortTernary.Found
• [ ] self-update.php file doesn't have a check for ABSPATH ... like defined( 'ABSPATH' ) || exit; at the top.
• [ ] Remove user-agent in the wp_remote_get options
• [x] Line 35 in self-update.php returns void but the function is marked as returning array|false
• [ ] There is no POT file in the repo
• [ ] Instead of define do a defined || define check to allow overwriting constants in wp-config
• [ ] The bootstrap file does not check that the "correct" version of PHP and WP are running
• [ ] BLOGROLL_OPTIONS variable name
• [ ] type-hint arguments and return values
• [ ] add declare( strict_types = 1 );
• [ ] Maybe Instead of enqueuing the styles/scripts inside the shortcode callback, enqueue them on the action wp_enqueue_scripts with the conditionals has_shortcode()