Problem/Motivation

The google_tag.container configuration entities have an incomplete configuration schema.

Proposed resolution

Fix the google_tag.container config entity schema.

Remaining tasks

  1. Write a patch
  2. Review
  3. Commit

User interface changes

None.

API changes

To be determined.

Data model changes

To be determined.

CommentFileSizeAuthor
#19 3069679-19.patch6.11 KBslasher13
#19 interdiff.txt1.75 KBslasher13
#12 Sélection_069.png20.08 KBsteveoriol
#5 interdiff.txt360 bytesaskibinski
#5 3069679-5.patch4.37 KBaskibinski
#3 3069679-3.patch4.37 KBaskibinski
#2 3069679-2.patch6.83 KBaskibinski
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

idebr created an issue. See original summary.

askibinski’s picture

askibinski CreditAttribution: askibinski at ezCompany for Erasmus University Rotterdam commented
Status: Active » Needs review
FileSize
3069679-2.patch6.83 KB
askibinski’s picture

askibinski CreditAttribution: askibinski at ezCompany for Erasmus University Rotterdam commented
FileSize
3069679-3.patch4.37 KB

Here is the same schema but for one schema.yml file. Seems the way core prefers it.

idebr’s picture

idebr CreditAttribution: idebr at ezCompany commented
Status: Needs review » Needs work 
_default_container:
  type: google_tag.container.[%key]

This works really nice!

Some remarks:

  1. +++ b/config/schema/google_tag.schema.yml
@@ -1,65 +1,64 @@
+    label:
+      type: string
+      label: 'Container label'

    Without any configuration schema, the label property is a type: label and is translatable. A string is not translatable, so this could lead to data loss.

  2. The patch should include an upgrade path for existing container configuration entities, see an example at https://www.drupal.org/node/2949630
askibinski’s picture

askibinski CreditAttribution: askibinski at ezCompany for Erasmus University Rotterdam commented
FileSize
3069679-5.patch4.37 KB
interdiff.txt360 bytes

Here's a new patch which keeps the type of the label property 'label' instead of string.

askibinski’s picture

askibinski CreditAttribution: askibinski at iO for Erasmus University Rotterdam commented
Status: Needs work » Needs review
idebr’s picture

idebr CreditAttribution: idebr at ezCompany commented
Status: Needs review » Needs work

Nice! Still needs work for #4.2

solotandem’s picture

solotandem CreditAttribution: solotandem commented

@idebr
On what basis do you mark this issue as a bug? What do you claim does not work with the current code?
You claim an "incomplete" configuration schema. How so?

@idebr, @askibinski
Why do either of you regard the suggested change as making the schema complete? Please elaborate on the rationale for the changes.

idebr’s picture

idebr CreditAttribution: idebr at ezCompany commented

Drupal Core's PHPUnit based automated tests check configuration data for the existence of a configuration schema and will trigger an error when a configuration schema is missing. As a result, developers cannot include any Google Tag Containers in their automated testing.

A more complete description of what the configuration schema is used for is available at https://www.drupal.org/docs/8/api/configuration-api/configuration-schema...

solotandem’s picture

solotandem CreditAttribution: solotandem commented

Again, that is a general statement. Would you provide evidence that it applies to this project? Also, would you reply to the other questions posed ?

idebr’s picture

idebr CreditAttribution: idebr at iO commented

Sure, I could write an automated test to show the current behavior if you like? I searched the issue queue if there was an open task for it but I could not find any.

You would have to enable testing on the 'Automated testing' tab on the project page for it to run on the Drupal.org testing infrastructure: https://www.drupal.org/node/2199963/qa

steveoriol’s picture

steveoriol
Location Grenoble 🇫🇷
CreditAttribution: steveoriol commented
FileSize
Sélection_069.png20.08 KB

Does this problem have something to do with the following error that I find in the page "/admin/reports/status" ?
Error in status

EdPhillis’s picture

EdPhillis CreditAttribution: EdPhillis commented

Yes, Im here like Steve, with the same error. Im guessing it is. Drush entup doesnt fix the problem like it normally does with mismatched entities.

solotandem’s picture

solotandem CreditAttribution: solotandem commented

@steveoriol, @olomouc
Are you running latest dev (or 8.x-1.2)? Did you run database updates?

miikamakarainen’s picture

miikamakarainen CreditAttribution: miikamakarainen commented

I have the same issue with mismatched entity/field definitions as Steveorio and Olomouc am running 8.x-1.2 (8.x-1.1 before update) and have ran database updates.

steveoriol’s picture

steveoriol
Location Grenoble 🇫🇷
CreditAttribution: steveoriol commented

@solotandem, I am in version 8.x-1.2 (8.x-1.1 before update), and I have also do run the drush updb ...

parisek’s picture

parisek
Primary language Czech
CreditAttribution: parisek commented

Had same problem with update to 8.x-1.2, resolved with module reinstall

steveoriol’s picture

steveoriol
Location Grenoble 🇫🇷
CreditAttribution: steveoriol commented

thanks parisek, it works for me too, it's just a pity to reinstall and configure for each site ...

slasher13’s picture

slasher13
Primary language German
CreditAttribution: slasher13 as a volunteer and at Wolters Kluwer commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.75 KB
3069679-19.patch6.11 KB

addressed #4.2 and #12

shepparddigital’s picture

shepparddigital CreditAttribution: shepparddigital commented

I applied slasher13's patch to a Drupal 8.7.7 site and it has resolved the problem. Thanks.

xeM8VfDh’s picture

xeM8VfDh CreditAttribution: xeM8VfDh commented

I'm here same as @steveoriol (#12), @olomouc (#13), and @miikamakarainen (#15)

I'm running Drupal 8.7.7 and GTM 8.x-12.

drush entity:updates does not work. When I run it it says: "The Container configuration entity type needs to be installed.". But, answering yes and going along with the installation doesn't resolve the issue. No errors are thrown, but when I try to update entitites again, expecting no more problems, the same message about having to install this GTM entity is shown. Answer yes again, again not fixed.

I have run database updates.

Is the solution to either reinstall or wait for this path to be released?

solotandem’s picture

solotandem CreditAttribution: solotandem commented
Status: Needs review » Needs work

This issue concerns the claim of an 'incomplete config schema' implying it is invalid and as a result something is not working as expected with the module code (i.e. the definition of a bug). This claim is unsubstantiated. Further, the suggested changes to the schema yaml files (in patches 2,3 and 5) would render the schema inconsistent with the implementing code in the project.

Comment #12 introduces a separate issue, namely the need to 'install' the 'google_tag_container' entity type. A separate issue has been created for this topic. See #3081007: Officially 'install' the 'google_tag_container' entity type. Comments 12-21 apply to the new issue. Any additional comments on this topic should be added to the new issue.

solotandem’s picture

solotandem CreditAttribution: solotandem commented
Assigned: Unassigned » solotandem
Status: Needs work » Fixed

In the process of writing tests I encountered the schema error mentioned in #9 and implied in the substance of the issue. Thank you all for reporting and commenting. That said I did not change the schema in all respects as suggested by the patch files. I retained the _google_tag_container type and its use on google_tag.settings. (This structure is consistent with the settings form code, what is stored in the config table, and does not cause errors in the tests.) We agree on adding the container properties to the google_tag.container.* schema. The latter changes make the schema consistent with the container entity definition and the container form. As an aside, I find it rather odd that Drupal does not complain about the schema except when running tests.

On the topic of 'installing' the container entity type, the google_tag.post_update.php code in patch #19 appears unnecessary as the existing configuration entities agree with the revised schema. In essence the saved container configuration agrees with the schema changes (i.e. the schema was out of sync with the entity). If I am missing something here and someone can demonstrate otherwise, please create an issue for same (or comment on issue 3081007 [intentionally not linked to again]).

The schema changes commit does not refer to this issue as it should. That was my oversight in getting into the test writing and remembering this issue afterwards.

Status: Fixed » Closed (fixed)

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