Allow disabling import of theme configuration

[ashmaroli]

• This is a 🙋 feature or enhancement.
• I've added tests.
• I've adjusted the documentation.

Summary

So that related bugs are not activated out-of-the-box..

Context

Resolves #8114

[DirtyF]

Can you explain how you see a theme config play with a site config?

I would expect the theme to have his own config, my site to have its config, and my site's config to override the theme's config if I want to override some theme's config.
A clear convention before adding options.

[ashmaroli]

Yes, that was exactly the intended workflow.
Only that are some unforeseen stuff that comes into play.
For example, say a user's theme's config resolves to:

{"foo"=>{"alpha"=>"beta"}}

And their site config includes the following pair:

{"foo"=>{"lorem"=>"ipsum"}}

The user would expect "foo" to just have a single attribute ("lorem"). But, in reality, the resulting config has "foo" with two attributes ("alpha" and "lorem"). The user has to then try and workaround this by explicitly setting foo.alpha => nil.
For a feature meant to ease onboarding, this additional overhead does not bode well.

Another unforeseen issue is that the resulting config (after overriding theme-config with user-end config) is a Ruby Hash instance rather than a Jekyll::Configuration (subclass of Ruby Hash) instance. So effectively, the query site.config.is_a?(Jekyll::Configuration) is false which was an unintended breaking change and another v4.0 regression.

The easiest route out I saw was to remove the feature entirely since it is just one-version old. But I guess SemVer principles expects us to carry the baggage until a v5.0. So the next route would be to either provide an option to disable this non-essential feature or add more technical debt (to buoy a non-essential feature) by patching the bugs as they are reported.

[DirtyF]

When there's no overlap between the theme and the site's config, do we get a Jekyll::Configuration?

[ashmaroli]

When there's no overlap between the theme and the site's config

We get a Jekyll::Configuration if nothing is done irrespective of overlap.
Consider the following lines on master:

jekyll/lib/jekyll/site.rb

Lines 434 to 436 in 18f04c6

	def load_theme_configuration(config)
	theme_config_file = in_theme_dir("_config.yml")
	return config unless File.exist?(theme_config_file)

The argument config to the above private method is always an Jekyll::Configuration instance.

jekyll/lib/jekyll/site.rb

Line 441 in 18f04c6

theme_config = SafeYAML.load_file(theme_config_file)

The result from SafeYAML.load_file is always a Hash

jekyll/lib/jekyll/site.rb

Line 450 in 18f04c6

Utils.deep_merge_hashes(theme_config, config)

And finally Hash is returned by the utility method

jekyll/lib/jekyll/utils.rb

Lines 28 to 30 in 18f04c6

	def deep_merge_hashes(master_hash, other_hash)
	deep_merge_hashes!(master_hash.dup, other_hash)
	end

jekyll/lib/jekyll/utils.rb

Lines 41 to 47 in 18f04c6

	def deep_merge_hashes!(target, overwrite)
	merge_values(target, overwrite)
	merge_default_proc(target, overwrite)
	duplicate_frozen_values(target)
	target
	end

[mattr-]

I used the docs as the authoritative version of what we want for this feature and made suggestions to the code based on that. That would also mean the title of the PR also doesn't match. 😄

[ashmaroli]

Thanks for the review @mattr-
I started this off with an opt-out stand and switched to an opt-in in a6f932e But I forgot to update the documentation.
Either ways, which stand would be the most apt for Jekyll 4.1 and Jekyll 4.0.1..? Opt-in or opt-out?

[mattr-]

Opt out is the least surprising from my point of view, as the feature was already enabled. We don't have numbers but I imagine the number of people that run into this is low enough that we can just point them to the docs when they bring it up and that's good enough for now. we can always revisit the default in 5.0

[DirtyF]

Opt out 👌

[ashmaroli]

@jekyllbot: merge +minor