Add functionality to automatically remove Content and Field Mappings

Kicking the testbot. This is only a partial implementation, but would like comments and implementation at least looked over if someone has the opportunity. Next stop - implement hook_field_delete_field.

Like we discussed, I would rather try to use rdf_get_mapping() if possible, and then act on that config entity.

What I didn't like about that was that entity_load, if there is no mapping, returns FALSE. rdf_get_mapping will return me an structured array that will sort of "start me a mapping" if one does not exist. So then I'm testing for some particular structured array rather than a simple "FALSE."

I'd be happy to use rdf_get_mapping, but I feel like we might want ot change the function signature to add a (..., $return_empty = TRUE) or something. Alternatively, we could write "rdf_mapping_exists" but all it would do is call the entity_load.

Other thoughts?

I think you had mentioned during our last sprint: we could also inspect the new empty mapping that rdf_get_mapping() returns, and detect that the mapping doesn't exist. I *think* I've seen the same logic of instantiating a new object in other APIs in core, but I don't remember which one it was (maybe config?).

1. I talked to Crell, who broke the tie. He agrees with you that if you can detect the empty case, better off calling the higher-level API over the lower one. I used the ->id property to detect if there was no mapping. So adjusted that code.

2. With help from chx, I was able to get pointed in the write hook direction for when a field instance is removed from an entity. In this case, there doesn't appear to really be a way to DELETE a field's mapping, only to set the mapping to empty. I don't love how this turns out in the yml when I config-export. It's still there, but set to an empty array. Not sure if we need a new function / API change.

Mostly writing a note to myself here with a status update from the last sprint...

This patch passes tests primarily because it doesn't test its own case, btw. The two functions, combined, break on the first $mapping->save call when deleting an entire bundle (deleting the bundle must also invoke the hook to delete the individual fields, which makes sense).

For some reason that mapping->save() call dies "accessing a non-object" -- will need to fire up the debugger and figure out the timings. Alternatively, the right approach here is probably to use a ->deleteMapping() call (new) rather than mapping->save()ing an empty array into the mapping. That alone might solve the issue.

And back to NW.

Added a deleteFieldMapping(). You still have to call save().

Started adding a test for "delete" to the crudTest.php, but maybe testing the automatic deletion of a mapping needs to be a web test (in order to trigger hook_ENTITY_TYPE_delete.)

Added a deleteFieldMapping(). You still have to call save().

This looks great, Jamie!

Started adding a test for "delete" to the crudTest.php, but maybe testing the automatic deletion of a mapping needs to be a web test (in order to trigger hook_ENTITY_TYPE_delete.)

Why is a web test required to trigger hook_ENTITY_TYPE_delete? Can't you delete the entity_test entity type in a unit test?

#2260457: Allow config entities to remove dependent configuration keys when dependencies are deleted due to module uninstall is related I think, and the ideas there to generalize deleting/cleaning-up related/dependant config entities when config entities are deleted. Right now, this deletion only happens when a module is uninstalled, and the cleanup there is also just done in that case. But we could possibly extend that to always use that system, instead of building multiple custom solutions.

Looking at RdfMapping::calculateDependencies(), it doesn't expose dependencies on defined fields yet, sounds like we should do that, similar to how EntityDisplayBase is doing it?

Let's try it. Note: we don't actually have a test for it here.

+++ b/core/modules/rdf/src/Entity/RdfMapping.php
@@ -147,6 +147,25 @@ public function calculateDependencies() {
+    // Create dependencies on both hidden and visible fields.
+    $fields = $this->content + $this->hidden;
+    foreach ($fields as $field_name => $component) {

Need to change the way I get the fields.

Berdir, would we need to wait for #2336727: Use configuration dependencies and onDependencyRemoval to standardise dependency management on configuration entity delete in order to use dependencies? and then implement something in onDependencyRemoval() that would only delete the mapping for just the field and not the whole bundle?

If you add the dependencies, then you need to implement onDependencyRemoval() too, or the whole rdf mapping would get dropped when a field is deleted as part of a module uninstallation.

Once you have that, you can either continue with custom code for now or wait (hope? ;)) for the other issue to happen.

Adds a test to #17. Not using dependencies for now. Just want to get the test working. But when I delete() the field, the delete hooks are not triggered.

Delete the instance. Then, the delete hooks get triggered.

Passed locally.

+++ b/core/modules/rdf/rdf.module
@@ -575,3 +576,34 @@ function template_preprocess_rdf_metadata(&$variables) {
+/**
+ * Implements hook_ENTITY_TYPE_delete() for 'field_instance_config'.
+ */
+function rdf_field_instance_config_delete(FieldInstanceConfigInterface $field_instance) {

This hook gets called when I run the test locally and all tests pass. But, on drupal.org, I can't tell if this hook is getting called. Appears not to be since the debug messages on 142/158 are the same.

Putting a debug() in the hook, to see if the hook gets called.

Result: It is not getting called. Reason why is in the next comment.

1.) Rerolled due to #2312093: Rename FieldInstanceConfig to FieldConfig.
2.) Removed a debug().
3.) Changed the Assert (post delete) to be more specific.

1. +++ b/core/modules/rdf/src/Tests/CrudTest.php
   @@ -7,6 +7,7 @@
   +use Drupal\rdf\Tests\Field\FieldRdfaTestBase;
    use Drupal\simpletest\DrupalUnitTestBase;
   

   Looks like we are no longer using DrupalUnitTestBase, so maybe this line could be removed?

2. +++ b/core/modules/rdf/src/Tests/CrudTest.php
   @@ -38,10 +39,29 @@ class CrudTest extends DrupalUnitTestBase {
   +  protected $fieldType = 'email';
   

   any particular reason for using email as opposed to a regular text field? email is probably fine too.

3. +++ b/core/modules/rdf/src/Tests/CrudTest.php
   @@ -112,4 +132,34 @@ function testFieldMapping() {
   +    debug($field_mapping);
   

   did you intend to leave this debug() here?

4. +++ b/core/modules/rdf/src/Tests/CrudTest.php
   @@ -112,4 +132,34 @@ function testFieldMapping() {
   +    // Now delete the field.
   

   I was confused initially when reading this comment. We're deleting the field value from the entity. We're not deleting the field per se yet.

5. +++ b/core/modules/rdf/src/Tests/CrudTest.php
   @@ -112,4 +132,34 @@ function testFieldMapping() {
   +    debug($field_mapping);
   

   debug()

Shouldn't we also test the bundle deletion? or is that tested elsewhere?

Made some of the changes. Added some comments.

Thanks Jamie. I'm happy with those changes. The only thing remaining now it to also test rdf_entity_bundle_delete().

+++ b/core/modules/rdf/src/Tests/CrudTest.php
@@ -112,4 +131,33 @@ function testFieldMapping() {
+    // No specific reason for choosing an email field.

that comment feels a bit out of place. I don't think we need to say that anywhere.

Tests automatic removal of the mapping by rdf_entity_bundle_delete().

+++ b/core/modules/rdf/src/Tests/CrudTest.php
@@ -38,10 +38,29 @@ class CrudTest extends DrupalUnitTestBase {
+    $this->createTestField();

@@ -112,4 +131,59 @@ function testFieldMapping() {
+    entity_test_create_bundle($this->bundle);
...
+    // Get the mapping again, now that it should have been deleted.
+    $field_mapping = rdf_get_mapping('entity_test', 'entity_test')
+      ->getFieldMapping($this->fieldName);

You should check that the entire bundle mapping is deleted, not just the particular field.

Not sure why I needed to create the getId() function.

Fixed a bug in calculatedependecies. With calling save() on the rdf_mapping in the field_config_delete hook, and without checking to see if the object is empty, we got a fatal error.

+++ b/core/modules/rdf/src/Tests/CrudTest.php
@@ -179,11 +179,11 @@ public function testAutoDeleteAfterBundleDelete() {
+

Remove extra line.

+++ b/core/modules/rdf/src/Entity/RdfMapping.php
@@ -153,7 +160,9 @@ public function calculateDependencies() {
+      if(!empty($bundle_entity)) {

fix coding standard.

was there a patch attached with your comments #36 and #37?

No, those were just statements; todos.

Fixed a merge conflict.

cleaned up blank spaces

I reviewed the code, it all looks good to me. I'm not super familiar with the coding standards for 8, but what I do know of Drupal standards was all there.

I reviewed the behavior in practice -- added/deleted a field, checked the exported config, added a mapping, deleted the field, exported, checked the mapping. Deleted the whole bundle, verified that it was gone.

I ran a the CRUD tests locally--all passed.

FEEDBACK
[1] If I have anything to add, it seems like we're creating a test entity in setUp() and adding one field to it. Then we're removing the field and ensuring there's no mapping on the entity. A more thorough test might be that we add TWO fields to the test entity, then delete just ONE of them, and check the mapping for the whole bundle. This gives us a little bit more test coverage to ensure we're not over-reaching on our delete.

[2] Also, I see that getId() was created for the purpose of a solitary test to call it. Do we really need that? Can't we use ->id like elsewhere, or is it a different kind of ID or something?

1.) done.
2.) I guess we don't need getId()

plus, removed comment mentioned in #31

getId() (and call to it) is still there... were you leaving it open for more discussion or should it be gone? I vote to lose it, personally.

I think I tried removing it and it seemed to work. But I somehow left out the change.

updated.

+++ b/core/modules/rdf/src/Tests/CrudTest.php
@@ -31,27 +31,46 @@ class CrudTest extends KernelTestBase {
-  protected $entityType;
+  protected $entity_type;

This should not be changed, it was fixed in #2381921: Clean-up RDF module test members - ensure property definition and use of camelCase naming convention to comply with the coding standards which recommends camelCase instead of under_score notations.

@lokapujya it's best practice to avoid RTBC'ing patches that contain code your wrote :) so that they are reviewed by someone neutral.

It is very likely that this will no longer be required very soon, see #2416409: Delete dependent config entities that don't implement onDependencyRemoval() when a config entity is deleted. That is completing the working that we started with automatic deletion of config based on dependencies.

I'd suggest to provide a test-only patch, that should still fail, and it will probably pass after the referenced issue landed.

I think we would still need to set up the dependencies in calculateDependencies().

In addition would we need to add the dependencies to existing .yml files? such as:
rdf.mapping.node.article.yml

First a reroll, but want to try using calculateDependencies().

Just the test.

Still test only, addressing issues with the tests.

Attempting to set up the correct dependencies.

1. +++ b/core/modules/rdf/src/Entity/RdfMapping.php
   @@ -147,7 +158,16 @@ public function calculateDependencies() {
   +      $mappings = $this->fieldMappings;
   +      $field_definitions = \Drupal::entityManager()->getFieldDefinitions($this->targetEntityType, $this->bundle);
   +      foreach (array_intersect_key($field_definitions, $mappings) as $field_name => $field_definition) {
   +        if ($field_definition instanceof ConfigEntityInterface && $field_definition->getEntityTypeId() == 'field_config') {
   +          $this->addDependency('config', $field_definition->getConfigDependencyName());
   +        }
   +      }
   

   Here, I am adding a dependence for each of the field Mappings. However, I only want to set the the dependency for the field that is currently being saved. How do I know which field is being saved? Is it even possible?

2. +++ b/core/modules/rdf/src/Tests/CrudTest.php
   @@ -38,10 +38,29 @@ class CrudTest extends KernelTestBase {
   +    $mapping->setFieldMapping($this->fieldName, array(
   +      'properties' => array('schema:email'),
   +    ))->save();
   

   Since the saving happens here. It is not actually saving the field mapping; it is saving the mappings for all fields of the bundle.

I may need to go back to the custom deleting of field mappings in #57.

Theoretically, by implementing onDependencyRemoval and returning TRUE, the rdf mapping should not get deleted. Instead, I remove the mapping for just the field that is getting deleted. However, it's still not working in my local testing.

Even if I just return True from onDependencyRemoval() and do nothing else, the Mapping for field 2 is still getting deleted.

Ok, so that fixes the problem. Needed to use $this instead of retrieving the mapping. The remaining 3 are side effects of adding calculateDependencies. No clue yet why.

StandardInstallerTest is failing. That sounds like something is could begetting deleted by calculateDependencies() unexpectedly. But the line that I'm editing in this patch seems to cause it.

interdiff for 77. this was a wild guess but didn't fix the problem.

1. +++ b/core/modules/rdf/src/Entity/RdfMapping.php
   @@ -147,13 +166,43 @@ public function calculateDependencies() {
   +    if (\Drupal::moduleHandler() && \Drupal::moduleHandler()->moduleExists('field')) {
   +      $mappings = $this->fieldMappings;
   +      $field_definitions = \Drupal::entityManager()->getFieldDefinitions($this->targetEntityType, $this->bundle);
   +      foreach (array_intersect_key($field_definitions, $mappings) as $field_name => $field_definition) {
   +        if ($field_definition instanceof ConfigEntityInterface && $field_definition->getEntityTypeId() == 'field_config') {
   +          $this->addDependency('config', $field_definition->getConfigDependencyName());
   +        }
   +      }
   +    }
   

   If I comment this block out, the standardInstall test doesn't fail. I only get the expected fail from the RDF crudtest.

2. +++ b/core/modules/rdf/src/Entity/RdfMapping.php
   @@ -147,13 +166,43 @@ public function calculateDependencies() {
   +      $mappings = $this->fieldMappings;
   +      $field_definitions = \Drupal::entityManager()->getFieldDefinitions($this->targetEntityType, $this->bundle);
   +      foreach (array_intersect_key($field_definitions, $mappings) as $field_name => $field_definition) {
   +        if ($field_definition instanceof ConfigEntityInterface && $field_definition->getEntityTypeId() == 'field_config') {
   +          $this->addDependency('config', $field_definition->getConfigDependencyName());
   +        }
   +      }
   

   If I comment the inner block, I still get tthe standardInstall test fails.

#80 was a lie. Got 3 errors, but they were different errors.

I think I understand what is happening now. CalculateDependencies() actually changes configuration; It adds dependencies to the field mappings. Then the StandardInstallTest diffs the deployed config which doesn't have the dependencies in the RDF Mappings. So, I think what needs to happen is that the RDF Mappings deployed .yml needs to have the dependencies. I suspected this, as mentioned way back in comment #59, but forgot about it.

Test out the theory above.

Good. That last patch fixed the errors that it was supposed to. There is still an issue with the bundle dependency. First of all, the test doesn't properly test to see if the bundle mapping gets removed. Then there are 2 PHPUnit test errors.

Update the delete bundle test.

Satisfy the PHPUnit tests.

The remaining fail is from a new test that I added and left out of the interdiff. Here is the interdiff for the new test. This test if for testing the rdf mapping dependency on the bundle. I needed to use an article, because the test case requires having an entity that manages it's bundles using entities. Here is an interdiff that shows the added test.

1. +++ b/core/modules/rdf/src/Entity/RdfMapping.php
   @@ -68,6 +69,16 @@ class RdfMapping extends ConfigEntityBase implements RdfMappingInterface {
      /**
   +   *  Return the id.
   +   *
   +   * @return string
   +   *   The Unique ID for the config entity.
   +   */
   +  public function getId() {
   +    return $this->id;
   +  }
   

   I don't see why this is needed or added here. Every entity has an id() method. just use that. It might not be a great name, but there's no need to duplicate that.

2. +++ b/core/modules/rdf/src/Entity/RdfMapping.php
   @@ -132,6 +143,14 @@ public function setFieldMapping($field_name, array $mapping = array()) {
      /**
       * {@inheritdoc}
       */
   +  public function deleteFieldMapping($field_name) {
   +    unset($this->fieldMappings[$field_name]);
   +    return $this;
   

   This is public and @inheritdoc but never defined anywhere on an interface. Does this really need to be public? If yes, it needs to be added and documented on the interface.

   Which is problematic because of BC. I would recommend to avoid this.

3. +++ b/core/modules/rdf/src/Entity/RdfMapping.php
   @@ -142,18 +161,49 @@ public function id() {
   +        if ($field_definition instanceof ConfigEntityInterface && $field_definition->getEntityTypeId() == 'field_config') {
   +          $this->addDependency('config', $field_definition->getConfigDependencyName());
   +        }
   

   not sure if this came up before but why limit it to field_config?

4. +++ b/core/modules/rdf/src/Entity/RdfMapping.php
   @@ -142,18 +161,49 @@ public function id() {
   +        // There may not be a mapping that needs to be adjusted.
   +        if ($this->getId()) {
   

   I don't understand this.

5. +++ b/core/modules/rdf/src/Entity/RdfMapping.php
   @@ -142,18 +161,49 @@ public function id() {
   +            $this->save();
   

   You're not supposed to save yourself. The API might just be checking if you could be changed or not.

6. +++ b/core/modules/rdf/src/Tests/CrudTest.php
   @@ -112,4 +131,110 @@ function testFieldMapping() {
   +  public function ptestAutoDelete() {
   

   this test is disabled?

1. ok
2. The method can be private.
3. That was copied from EntityDisplayBase. Seems like it's not needed here.
4. Neither do I. It was copied from somewhere.
5. Thanks!
6. Oops.

How do you delete a bundle?
This is what the test is trying, but I'm not sure this is right:

+++ b/core/modules/rdf/src/Tests/NodeAttributesTest.php
@@ -94,5 +94,40 @@ function testNodeAttributes() {
+    // Now delete the bundle.
+    $bundles = \Drupal::state()->get('node.bundles') ?: array('node' => array('label' => 'Article'));
+    unset($bundles['article']);
+    \Drupal::state()->set('node.bundles', $bundles);
+    \Drupal::entityManager()->onBundleDelete('article', 'node');

Try a different way of deleting the bundle.

The test is failing on 8.2.x. Something changed since this was written for 8.0.x.

Re-roll.

With rdf being removed in D10 wondering if this is still an issue?

RDF is moving out of core and into https://www.drupal.org/project/rdf.

To keep the ball moving closing this as outdated since this moved to PNMI. If still a valid bug please reopen under the contrib module.