Problem/Motivation

config_log 4.0.x declared two new settings (leading_context_lines and trailing_context_lines, added by drupal.org/project/config_log/issues/3525969) with `type: number`. `number` is not a defined Drupal core config schema primitive, so the strict ConfigSchemaChecker used by every Functional / BrowserTestBase run throws SchemaIncompleteException as soon as config_log is installed:

Drupal\Core\Config\Schema\SchemaIncompleteException:

Proposed resolution

In config_log.schema.yml, change leading_context_lines and trailing_context_lines to specify a type of integer, not number.

Issue fork config_log-3606794

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

ebeyrent created an issue. See original summary.

dkmishra made their first commit to this issue’s fork.

dkmishra opened merge request !29

dkmishra’s picture

dkmishra commented
Status: Active » Needs review
nagy.balint’s picture

nagy.balint commented
Status: Needs review » Needs work

Thank you for the report!

I think however that that langcode is not needed there, as it is likely inherited from core already.

dkmishra’s picture

dkmishra commented

Hi @nagy.balint, thanks for the feedback. Do you mean the explicit langcode mapping in config/schema/config_log.schema.yml, or the langcode: en value in config/install/config_log.settings.yml / the update hook?

nagy.balint’s picture

nagy.balint commented

I mean if I'm right that langcode is inherited, then we dont need to add it anywhere since the schema will be handled by core.

dkmishra’s picture

dkmishra commented
Status: Needs work » Needs review

Thanks, you’re right that the langcode schema mapping is inherited from core via type: config_object, so I removed the langcode entry from config/schema/config_log.schema.yml. Please review.

ebeyrent’s picture

ebeyrent commented
Status: Needs review » Reviewed & tested by the community

Tested and deployed, no issues.

nagy.balint’s picture

nagy.balint commented
Status: Reviewed & tested by the community » Needs work

Sorry but I still see langcode in the diff, while if it's inherited it should not be in the patch at all.

dkmishra’s picture

dkmishra commented

Hi @nagy.balint Thanks for the review.

I agree that langcode should not be declared explicitly in the schema because it is inherited from config_object, and I've already removed that part.

However, I also tested removing langcode: en from config/install/config_log.settings.yml. After doing so, drush config:inspect --detail --only-error reports:

----------------------- --------- ------------- ----------------------------------------------------------------------------------------- 
  Key                     Status    Validatable   Data                                                                                     
 ----------------------- --------- ------------- ----------------------------------------------------------------------------------------- 
  config_log.settings     Correct   56%           1 errors                                                                                 
   config_log.settings:   Correct   Validatable   The config_log.settings config object must specify a language code, because it contains  
                                                  translatable values.                                                                     
 ----------------------- --------- ------------- -----------------------------------------------------------------------------------------

This suggests that while the schema inherits langcode, the configuration object itself still requires a langcode value. For that reason, I believe keeping langcode: en in the install configuration (and adding it for existing installations via the update hook) is the correct approach.

Could you please verify this on your side as well? If I'm overlooking something, I'd be happy to update the patch accordingly.

nagy.balint’s picture

nagy.balint commented
Status: Needs work » Reviewed & tested by the community

Upon further review, I think you are right!

It has to remain in install.

Of course it would have been better with a single update hook, especially as the 8003 saves unconditionally, but its a minor thing.

nagy.balint’s picture

nagy.balint commented
Status: Reviewed & tested by the community » Fixed

Thank you!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.