Problem/Motivation

In order to move the RDF module from Core to Contrib, we need to move all RDF-module related tests to the RDF directory/namespace.

Steps to reproduce

Proposed resolution

Remaining tasks

Investigate EntityViewControllerTest, see #6

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3293813

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Spokje created an issue. See original summary.

Spokje’s picture

Spokje’s picture

Status: Active » Needs work

Work in Progress ATM.

quietone’s picture

Applied the MR. I ran the failing test locally and it passed. Then I searched for rdf in tests while specifically ignoring migration related tests.

$ grep -riw -I rdf core | grep -v core/modules/rdf | grep -v migrate | grep Test.php | awk -F: '{print $1}' | grep -v grep | sort -u | nl
     1  core/modules/hal/tests/src/Functional/rdf/RdfMappingHalJsonAnonTest.php
     2  core/modules/hal/tests/src/Functional/rdf/RdfMappingHalJsonBasicAuthTest.php
     3  core/modules/hal/tests/src/Functional/rdf/RdfMappingHalJsonCookieTest.php
     4  core/modules/system/tests/src/Functional/Common/NoJavaScriptAnonymousTest.php
     5  core/modules/system/tests/src/Functional/Entity/EntityViewControllerTest.php
     6  core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathTestBaseFilledTest.php
     7  core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyCopiesTest.php

What about these tests? Is there anything to move?

longwave’s picture

Re #5

I think the 3 HAL test can be deleted when we remove RDF module. It seems super unlikely anyone is ever reading RDF mappings via HAL, given that both those modules are on their way out of core.

NoJavaScriptAnonymousTest refers to all modules in standard so that should be dealt with in #3243121: Remove RDF module from the Standard profile

EntityViewControllerTest has a small test method that probably needs porting to the RDF module.

UpdatePathTestBaseFilledTest tests the upgrade path from 9.3.0 so references to RDF will be removed there when we remove the module itself.

ConfirmClassyCopiesTest needs to stay while rdf-metadata.html.twig remains in the core themes.

To summarize I think only EntityViewControllerTest needs investigation here.

quietone’s picture

Issue summary: View changes
Spokje’s picture

Assigned: Spokje » Unassigned
Issue summary: View changes
quietone’s picture

Issue summary: View changes

Add remaining task to IS.

quietone’s picture

I have looked at EntityViewControllerTest and I don't know how to modify it so test coverage is kept and RDF is removed.

Anyone? I'll have a go if someone points the way.\

catch’s picture

The code in RDF that's being relied on is this (I think):

 * Implements hook_entity_prepare_view().
 */
function rdf_entity_prepare_view($entity_type, array $entities, array $displays) {
  // Iterate over the RDF mappings for each entity and prepare the RDFa
  // attributes to be added inside field formatters.
  foreach ($entities as $entity) {
    $mapping = rdf_get_mapping($entity_type, $entity->bundle());
    // Only prepare the RDFa attributes for the fields which are configured to
    // be displayed.
    foreach ($displays[$entity->bundle()]->getComponents() as $name => $options) {
      $field_mapping = $mapping->getPreparedFieldMapping($name);
      if ($field_mapping) {
        foreach ($entity->get($name) as $item) {
          $item->_attributes += rdf_rdfa_attributes($field_mapping, $item->toArray());
        }
      }
    }
  }
}

So what we'd need is a test module that sets $item->_attributes to something, and change the assertions to look for that instead.

quietone’s picture

Status: Needs work » Needs review

@catch, thanks!

I made a new test module and used the same hook to add the same attribute.

longwave’s picture

Status: Needs review » Needs work

Looking at EntityViewControllerTest again, I would argue that this piece of functionality is no longer worth testing.

EntityViewBuilder sets:

            $item->_attributes = [];

entity_test.module appends to this array:

          $item->_attributes += ['data-field-item-attr' => 'foobar'];

entity_view_controller_test also appends to this array:

        $item->_attributes += ['property' => 'schema:text'];

So really all we are doing is checking that PHP can correctly merge arrays. When RDF was involved I guess it was important to ensure that RDF didn't accidentally overwrite existing attributes, but there is nothing to stop a module overwriting $item->_attributes instead of merging, so the test proves nothing to me.

longwave’s picture

Otherwise the changes look fine to me. NW for #13 (or just to update the .info.yml if you disagree with #13).

longwave’s picture

Found via #3267703: Deprecate RDF module I think JsonApiRegressionTest::testLeakedCacheMetadataViaRdfFromIssue3053827() also needs moving to RDF module as an edge case in JSON API.

Spokje’s picture

Found via #3267703: Deprecate RDF module I think JsonApiRegressionTest::testLeakedCacheMetadataViaRdfFromIssue3053827() also needs moving to RDF module as an edge case in JSON API.

@longwave: That is already in the MR and moved to the RDF module/namespace?
https://git.drupalcode.org/project/drupal/-/blob/c35932f73069b437e1995e4...

longwave’s picture

Status: Needs work » Needs review

Sorry, I am blind, don't know how I didn't see that :)

Addressed #13 by removing the test.

longwave’s picture

In fact we can keep the spirit of the original tests by ensuring that the entity_test module adds two attributes, and then testing for both of them.

longwave’s picture

Unsure if we should decouple migrate_drupal_ui from RDF in here or in #3267703: Deprecate RDF module, the latter forces us because the tests fail with a deprecation error.

Spokje’s picture

Unsure if we should decouple migrate_drupal_ui from RDF in here or in #3267703: Deprecate RDF module

In previous deprecation/removal "adventures" we had a separate issue for "Move migrate related [insert_module_name] tests to the module in preparation of removal in d10". (See for example #3265424: Move migrate related aggregator tests to the module in preparation of removal in d10) where this was done.

For RDF, this happened already in #3267513: Handle migration tests for removing RDF, but we re-added it again in #3243121: Remove RDF module from the Standard profile.

So it seems we worked a bit backwards there and we can/should remove 'rdf' from all the protected static $modules arrays in migrate_drupal_ui.

Note: I'm not telling this to educate @longwave, he almost certainly got to the above long before I did. Just wanted to document it for my sanity.

quietone’s picture

In general, I think it is best to have changes to migration tests in a separate issue so that only those changes are considered. Experience is showing that is difficult and we have made some mistakes. However, in this particular case lets update the migrate_drupal_ui tests here and not muddle up the deprecation issue. I have moved the changes to migrate_drupal_ui from #3267703: Deprecate RDF module to here.

catch’s picture

Agreed with @quietone in #21 those migrate_drupal_ui changes look fine to include here since it's a pretty trivial change (unlike many others).

nod_’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Looks like it's all there. Just a nitpick on a comment. Issue with EntityViewControllerTest has been resolved as far as I can see, removing from IS.

I don't feel too confident about knowing the ins and outs of the problem space but from what I can see patch does what the issue summary says it should do, so RTBC :)

longwave’s picture

Status: Reviewed & tested by the community » Needs review

Removed out of scope comment changes in the data provider, fixed two uses of ableist language in entity_test - while only one is technically in scope if we are going to fix one I think we should fix the other at the same time.

xjm’s picture

Status: Needs review » Needs work
longwave’s picture

Status: Needs work » Needs review
bbrala’s picture

Status: Needs review » Reviewed & tested by the community

Went through the changes, searched for 'rdf' to see any missing places where the module is enabled. Ran RDF tests on the MR locally and they are green. I'd say, RTBC ^^

  • xjm committed 555f445 on 10.1.x
    Issue #3293813 by Spokje, longwave, quietone, catch, nod_, bbrala, xjm:...

  • xjm committed fe19730 on 10.0.x
    Issue #3293813 by Spokje, longwave, quietone, catch, nod_, bbrala, xjm:...

  • xjm committed 50737e4 on 9.5.x
    Issue #3293813 by Spokje, longwave, quietone, catch, nod_, bbrala, xjm:...

  • xjm committed d95fb87 on 9.4.x
    Issue #3293813 by Spokje, longwave, quietone, catch, nod_, bbrala, xjm:...
xjm’s picture

Version: 9.5.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed to 10.1.x, 10.0.x, 9.5.x, and 9.4.x as a patch-eligible test cleanup. Thanks!

  • xjm committed 883d501 on 9.4.x
    Revert "Issue #3293813 by Spokje, longwave, quietone, catch, nod_,...
xjm’s picture

I think this may have introduced a fail on 9.4, so reverting there.

Status: Fixed » Closed (fixed)

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

AdamPS’s picture

This issue seems to have removed a lot of important testing from Drupal Core: NodeDisplayConfigurableTest, node_display_configurable_test.info

This testing (that I spent quite a while writing) is/was an important part of the initiative #2353867: [META] Expose Title and other base fields in Manage Display. I have raised #3342700: Reinstate important testing NodeDisplayConfigurableTest. I hope that the authors of this issue might be willing to undo this accidental loss from their actions.