Could also improve this to be

Jekyll::External.require_with_graceful_fail(site.gems.select { |g| plugin_allowed?(g) })

What do you think?

That looks like a cleaner change. If we want to one line this, is there still a point to having the require_gems method? It's only used on line 19 (PluginManager#conscientious_require) so we could just use this one-liner there? How do we feel about that? Something like:

    def conscientious_require
      require_plugin_files
      Jekyll::External.require_with_graceful_fail(site.gems.select { |g| plugin_allowed?(g) })
      deprecation_checks
    end

I also notice that require_plugin_files is still using require without the graceful_fail method.

All this debate is a smell, that file needs to be reviewed.

is there still a point to having the require_gems method?

Absolutely! Code clarity. This whole thing is for GitHub after all, for their security team to ensure that non-whitelisted gems are not required. So if we can encapsulate this as its own method, it's easier to audit (and to understand that line's purpose).

I also notice that require_plugin_files is still using require without the graceful_fail method.

Good point. You can fix that up too if you would like.

Okay, valid point, makes sense. I do notice that this would remove the Jekyll.logger.debug if we wanted to one-line it. Would this break backwards compatibility if anyone is asserting or relying on these debug messages?

Also, thanks for the quick feedback!

We make no guarantees, so they should not rely on it. Is there a debug message in require_with_graceful_fallback?

Nope, only a logger.error message about the Dependency Error in case of LoadError in require_with_graceful_fail.

Think it would be valuable to include a Jekyll.logger.debug message about which gem is being required in require_with_graceful_feedback, provided that we remove any redundant logger.debug messages in places that use require_with_graceful_fail. And looking through the codebase, there don't seem to be any.

Make it so!

Let's do Utils.safe_glob(plugin_search_path, File.join("**", "*.rb")) here instead :)

Sure, that sounds good. Is there still a need for using File.join if we're passing in explicit parameters for the call? (i.e. it'll always just return "**/*.rb"

@RochesterinNYC Yes, the File.join returns "**/*.rb" on Linux and Mac, but will return "**\*.rb" on Windows. :)