Add undeclared variables to the Feed entity class

RE: #12

This also applies to the undeclared $feed->items property.

I'm sensing some code smell from these undeclared variables. It's more than just a bad practice in general. They obfuscate dependencies of the parser and processor plugins. Why doesn't fetch() return the body of the response instead of assigning it to $feed->source_string? Why doesn't parse() return the items instead of assigning it to $feed->items? I do not like using an entity class like this.

I want to remove the use of these variables altogether. But doing that would break backward compatibility by changing the plugin APIs. I don't know that I want to bother with releasing version 3 right now. Also, I feel like we should get a version that doesn't trigger deprecation notices in PHP 8.2 out sooner rather than later.

Suggested plan:

• Add the three variables as protected to the Feed class.
• Add the __get function to return the variables. Have the function trigger a deprecation notice stating that they will be removed in version 3.0.0.
• Open issues for removing the three variables and changing the plugin APIs to not use them. I suggest keeping the issues for each variable separate.

Please provide feedback.

Here's a patch for the plan I outlined in #16. Though I decided to not do anything with $http_headers and just let it be deleted as in the original patch. This may not be the correct thing to do. Feel free to comment.

The last submitted patch, 17: 3239690-17.patch, failed testing. View results →

An empty($feed->items); call in the ItemsImporter caused the failures above. empty() calls the __isset() magic method, which hadn't been implemented. I could have rewritten that line to check for the existence of items in the array a different way, but I figured a more complete solution is to implement __isset().

I also removed a couple of other dynamic variable assignments in AggregatorTestBase. One was for $feed->items, but used it in a completely different way that the importers, which I couldn't believe. The other was for yet another new dynamic variable. Since it's only in a test I've chosen to not add it to the Feed class.

The last submitted patch, 19: 3239690-19.patch, failed testing. View results →

My change to remove those additional dynamic variables in the test base broke two of the tests. They were actually using that badly-assigned items variable. So I had to write it out of those other tests.

+++ b/src/Entity/Feed.php
@@ -55,6 +55,67 @@ use Drupal\aggregator\FeedInterface;
+    if ($name == 'items') {
+      @trigger_error('The $items property is deprecated in 2.1.0 and will be removed from 3.0.0. See https://www.drupal.org/project/aggregator/issues/3239690.', E_USER_DEPRECATED);
+      $this->items = $value;
+    }
+    elseif ($name == 'source_string') {
+      @trigger_error('The $source_string property is deprecated in 2.1.0 and will be removed from 3.0.0. See https://www.drupal.org/project/aggregator/issues/3239690.', E_USER_DEPRECATED);
+      $this->source_string = $value;
+    }
+    else {
+      parent::__set($name, $value);
+    }

Ubernit, but personally I see elseif (and else) as a code smell.

We can return early here and just use if.

I think we're supposed to link to a change record rather than an issue node - we can create one here: https://www.drupal.org/node/add/changenotice?field_project=3262413&field... →

Not sure this is worth fixing, feel free to ignore on both counts

Here's the change record: https://www.drupal.org/node/3386012 → .

I also edited the __set() function as suggested.

Thank you to everyone who worked on this issue and the one that was closed as a duplicate!

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