Refactor aggregator to use processed_text

Rerolled #8.

The last submitted patch, 34: 2552495-34.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

It helps to include the new config.

The last submitted patch, 36: 2552495-36.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

I missed a change to FeedViewBuilder.

The last submitted patch, 38: 2552495-38.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

I haven't fixed all of the errors yet, but I want to upload this work before I leave on vacation just in case someone else happens to pick it up. I'm not sure why the /aggregator/sources view isn't being installed properly. Nor do I know why it's throwing database serialization errors. I'm worried that it has something to do with the addition of the logger to AggregatorFeedBlock in 🐛 Bad link in aggregator crashes production website Fixed .

The last submitted patch, 40: 2552495-40.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

This patch will also fail. I fixed the migration test errors weeks ago, but I still don't know why the AggregatorRendering test is broken. When I check the HTML output a 404 response was returned. But I can verify with additional assertions that the view config is installed. It just doesn't work. What's worse is that when I install the module on the Minimal profile the whole site breaks. Suddenly classes can't be found like something went wrong with registering them. I have no idea why this is happening. It doesn't seem like these changes should cause that sort of thing to occur.

My plan is to try recreating the patch's changes one-by-one until the site breaks again. Hopefully that will expose the problematic change. But I need to save my current work first.

Let's try this again.

The last submitted patch, 43: 2552495-43.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The MigrateAggregatorSettingsTest failure is fixed simply by adding 'filter' to the test's list of installed modules. But I'm not going to upload a new patch just for that when I know the others are still going to fail.

The user roles listed in the filter format config caused the problem, probably because one or both of them don't exist in the testing profile. Rather than try to add them I decided to delete them. After all, we don't offer an interface for editing Item entities. No one should ever really be using the format other than Aggregator itself. If an admin wants to add access, then that's their call. But we don't need to enable it for any roles by default.

The last submitted patch, 46: 2552495-46.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Unless I'm mistaken, this will fix the last of the test failures.

This should take care of the migrations.

1. +++ b/tests/src/Kernel/Views/AggregatorItemViewsFieldAccessTest.php
   @@ -27,6 +32,7 @@ class AggregatorItemViewsFieldAccessTest extends FieldFieldAccessTestBase {
        $this->installConfig(['aggregator']);
        $this->installEntitySchema('aggregator_feed');
        $this->installEntitySchema('aggregator_item');
   +    $this->installConfig(['filter', 'aggregator']);
   

   minor: We're installing aggregator config twice here.

2. +++ b/tests/src/Kernel/Views/IntegrationTest.php
   @@ -61,6 +63,7 @@ class IntegrationTest extends ViewsKernelTestBase {
        $this->installConfig(['aggregator']);
        $this->installEntitySchema('aggregator_item');
        $this->installEntitySchema('aggregator_feed');
   +    $this->installConfig(['filter', 'aggregator']);
   

   Here too

I think we might need an update hook to unset the allowed_html key in the config object?

Thank you for looking over the current patch!

I wrote the update hook this morning. It creates the filter format config and then deletes the old allowed_html setting. I'm working on the update test right now.

I fixed the issues mentioned in #50. I also added the update hook. But I'm having a hard time creating a fixture for the upgrade path test. I'm still working on it, but I want to save what I've done so far.

I'm embarrassed at how long it took me to finish writing this update path test. I had a character encoding problem in the fixture I created that caused weird failures. It took hours and hours of debugging to find the cause.

Anyway, this patch adds the upgrade path test. It should come back green, I think.

This is a reroll without the fixture, which was committed in 📌 Add a fixture for database update tests Fixed . I'm not going to bother with an interdiff since that was the only change.

I was going to comment that we need something to prevent filter.format.aggregator_html.yml being deleted.

But then I saw that \Drupal\filter\FilterFormatAccessControlHandler::checkAccess already prevents deletion via the UI and over APIs

So the only way to delete it is via a config import. Looking at other things in core, we would probably need a subscriber that extends from \Drupal\Core\Config\ConfigImportValidateEventSubscriberBase to prevent that.

Happy to defer that to a follow up to keep the scope smaller here because that's an existing issue in core - you could delete a filter format that was in use in content this way too.

Looking at \Drupal\filter\Element\ProcessedText::preRenderText the impact would be that the markup is just rendered empty, so there's no security impact. So we can probably just defer that to a minor task and not worry too much about people doing the wrong thing with their config.

Speaking as someone who has deleted a format that is in use, yeah it just makes all the content blank. For editable content you would just select another format, then save. Everything is fine after that. That isn't possible with Items, but an admin could restore the format's YAML file and re-import it to fix them.

Since we have several open issues with update hooks I'm thinking that it would be best to wait to commit this so that it's easier to apply other patches while reviewing.

#54 had to be rerolled due to a lot of recent changes. I also combined the update test with the recently-added update test for the removal of items.teaser_length in order to reduce code duplication and the number of functional tests.

Thank you to everyone who worked on this issue!

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