Remove aggregator module and our dependency on Laminas Feed

It should now be possible to update the PR to remove aggregator from the 10.x branch, now that #3264122: Move all non migration aggregator tests to the module in preparation of removal in d10 is in, and hopefully get a green test run.

Actual commit needs #3267458: Deprecate aggregator module in Drupal 9.4 to also be ready to go, which is currently blocked on a 9.4.x backport of #3264122: Move all non migration aggregator tests to the module in preparation of removal in d10.

All of the themes (plus umami) all still have an aggregator-item template and an aggregator-feed template. Is that an oversight? A quick search didn't lead me to any answers.

Other than that there's no more mention of aggregator outside of the d6/7 migrate test fixtures (which is fine) and some related tests that expext Aggregator to no longer be installed (which is good).

All the removals in the MR look intentional and correct as far as I can tell.

I just have that question about the aggregator-item and aggregator-feed templates.

1. phpstan baseline needs to be updated: https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/phpstan-bas...

I guess, this is not reported by phpstan because we run it on a partial code base, and here is another evidence why phpstan is supposed to run on the whole codebase

2. there are also mentions in https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/core.api.ph...

3. what about core/modules/migrate_drupal/tests/fixtures/drupal6.php and core/modules/migrate_drupal/tests/fixtures/drupal7.php

4. there are tests in block module, like https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/modules/blo...

5. there are test in migrate_drupal, like https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/modules/mig...

6. and a lot of templates within various themes

I can help sort through the comments in #15, particularly the migrate ones.

15.1. I have no expertise here.

15.2. That's a good catch. I missed that somehow. This should get updated.

15.3. Those fixtures are for d6 and d7 sites that have aggregator installed. That's fine. Aggregator still exists in D6 and D7. The fixtures will not be updated for any core modules being moved to contrib. No action needs to be taken here. For details, please see #3264607: [policy, no patch] How to ensure migrations continue to work when modules are moved to contrib.

15.4. The tests in the block module, much like the migrate test fixtures, are correct to continue to mention aggregator. It is key to note that the tests are not in fact mentioning the aggregator module. They are instead testing how an aggregator block gets migrated, which is a subtle but important distinction. No action needs to be taken here. For details please see #3265483: Handle block migration for modules moved to contrib.

15.5. The link here appears to be incorrect. I do not see aggregator mentioned in StateFileExistsTest in the MR. There are several mentions of "Aggregator" in migrate_drupal_ui tests, but this is only in the context of MigrateUpgradeTestBase::getMissingPaths. That is correct and does not need any attention.

15.6. I think that "a lot of templates within various themes" is the same comment I made in #14 regarding the aggregator-item and aggregator-feed templates. I don't believe there are any other template types that refer to aggregator.

hi @danflanagan8, thanks for the clarifications,

15.1 Those can and should be just removed

15.3/15.4 Got it

15.5 I was looking on outdated diff, sorry

15.6 I had a browser tab opened for a while, so I have missed your comment. Yes, those are the same template I believe.

All of the themes (plus umami) all still have an aggregator-item template and an aggregator-feed template. Is that an oversight?

Seems like an oversight, we should be able to just go ahead and remove all of those here.

OK, the last thing that needs to be updated is mentions in 15.2 /core/core.api.php and it is RTBC

hmm I had assumed it was an oversight but Spokje makes some good points.

A possible compromise would be to keep the templates in themes we're planning to move to contrib like Bartik, since the contrib version of the theme could/should support contrib modules and it'll be removed from core with them.

I don't think Olivero or Umami should explicitly support contrib modules. Maybe we should open a follow-up for this one?

I've checked templates, and

aggregator-feed.html.twig has two versions:

module/stable/stable9:

{{ title_prefix }}
{% if title and not full %}
  <h2{{ title_attributes }}>{{ title }}</h2>
{% endif %}
{{ title_suffix }}

{{ content }}

demo_umami/bartik/claro/classy/seven/starterkit_theme provides an extra div with a class:

<div{{ attributes.addClass('aggregator-feed') }}>

  {{ title_prefix }}
  {% if title and not full %}
    <h2{{ title_attributes }}>{{ title }}</h2>
  {% endif %}
  {{ title_suffix }}

  {{ content }}

</div>

aggregator-item.html.twig has two versions as well:

module/stable/stable9:

<article{{ attributes }}>
  {{ title_prefix }}
  {% if title %}
    <h3>
      <a href="{{ url }}">{{ title }}</a>
    </h3>
  {% endif %}
  {{ title_suffix }}
  {{ content }}
</article>

demo_umami/bartik/claro/classy/seven/starterkit_theme adds an extra class:

<article{{ attributes.addClass('aggregator-item') }}>
  {{ title_prefix }}
  {% if title %}
    <h3 class="feed-item-title">
      <a href="{{ url }}">{{ title }}</a>
    </h3>
  {% endif %}
  {{ title_suffix }}
  {{ content }}
</article>

This is so minor, that can be easily added in a change record - so custom themes could add them as part of the migration process

We asked @ckrina about this as a frontend framework manager. Her reply:

My rationale for _not_ deleting theme templates (it wasn’t an oversight) was that it doesn’t “hurt” to have them around

If we have them in core we’re responsible of them, meaning we need to be sure they keep working, that they are accessible… And if the module is moved to contrib we need to install a contrib module to test them. Unless I’m missing something, I’d move the templates to the module itself and delete them for the themes staying in D10, but I’m not sure how to handle the transition until we deprecate the other ones.

Discussed this with ckrina in slack a bit more, came up with this:

Status quo of the MR prior to template removal ws:
- it leaves all the templates where they are in all themes
- removes the template from the stable copies test (because it's impossible to test a template for a module that doesn't exist)

I think we should change it to:

- remove the templates from themes that are staying in core, leave them in deprecated themes
- still don't test the template in the stable copies test

This means we have some untested templates in those themes, but they won't change and they're tested in 9.4.x until that's out of support, and only until those themes are deprecated and removed, at which point the contrib versions will have the template and not regress compared to when they were in core.

So we should remove the templates from Umami, Claro, StarterKit, but leave them everywhere else.

OK I'm not 100% sure I got the list of themes right, or that I removed/restored the templates from the right ones even if I did, but something like this.

hi @catch, stable/stable9 has exactly the same templates as module provides -> no need to keep them imho.

Also, core.api.php still has references/examples of non-existing aggregator module.

hi @catch, stable/stable9 has exactly the same templates as module provides -> no need to keep them imho.

When we move stable/stable9 to contrib, it'll then keep the same templates for the module, which in theory might change them, so this is by design I think.

Have removed references to aggregator from core.api.php, but left the hook examples intact with more generic naming.

The test failure is because now that the templates are back in classy/stable, they get tested, and because the test no longer contains the hashes, it fails because it looks like we don't have coverage, which we don't.

I'm going to try restoring the test coverage - I think we need to just go with what's most pragmatic here, which might mean a follow-up to remove those lines from the test once the last theme with an aggregator template in it has been deprecated.

OK that looks like the right fix to me - just could not see that aggregator template in Claro because I failed to look in the dataset folder (or apparently grep properly for it).

I'm going to be bold and RTBC this. Everything @Taran2L and I discussed in #14 through #18 has been addressed. And the plan for the templates described in #27 appears to have been followed.

When I search core for aggregator I only find it in the context of

1. Migrate fixtures
2. Migrate tests where it makes sense to mention it
3. Templates in themes that are going to move to contrib (classy, seven, bartik, stable/stable9)
4. A handful of places where aggregator is used in a general English sense rather than referring to the aggregator module.

Looks good to me as well

Test failure is a known random so I requeued it.

Committed 0e2ea43 and pushed to 10.0.x. Thanks!

It looks like the updated database dumps added by this issue are not compatible with PostgreSQL and SQLite:

https://www.drupal.org/pift-ci-job/2349942

We may need to revert to address that, although I don't know what the fix is. Issues that change DB things should run tests against all databases before commit.

This is #3261629: Database dumps are no longer driver-agnostic.

The problem is that while that fix has been committed, it wasn't committed before 9.3.0 was released, and when people re-dump the database, they have 9.3.0 checked out.

To fix (hopefully) the dumps, this is what I did:

gunzip core/modules/system/tests/fixtures/update/drupal-9.3.0.filled.standard.php.gz

Edited it and manually added the if statements from #3261629: Database dumps are no longer driver-agnostic (top and bottom of the file).

gzip core/modules/system/tests/fixtures/update/drupal-9.3.0.filled.standard.php

We should try to find a way to have standard steps to generate the dumps that don't reintroduce this issue though - it might been copying the dump scripts from 9.4.x or 10.0.x out of the git tree before switching to 9.3.0, or loading the database into 9.3.0, making changes, then switching to 9.4.x (or 9.3.whatever) before running the dump script. Or a time machine to apply the commit to 9.3.0

Missing parentheses...

Committed the hotfix in #43 from NR to fix HEAD. Thanks @catch!

Tagging "Needs followup" for docs fixes about the DB dump process and the issue with DB drivers. Probably we should file an issue to discuss the best solution; see #42.

Added change records and tweaked the release note snippet.

Follow up made, #3305003: Create standard steps for creating database dumps, removing tag.