Absolute URL Plugin

$result_link

Yes, I believe that $result->link is gone. In D7, only some parsers set this value, like the syndication parser for example:

class FeedsSyndicationParser extends FeedsParser {
  public function parse(FeedsSource $source, FeedsFetcherResult $fetcher_result) {
    // (...)
    $result->link = $feed['link'];

In the D7 of Feeds extensible parsers it was taken from the fetcher's config:

abstract class FeedsExBase extends FeedsParser {
  public function parse(FeedsSource $source, FeedsFetcherResult $fetcher_result) {
    // (...)
    // Set link.
    $fetcher_config = $source->getConfigFor($source->importer->fetcher);
    $result->link = isset($fetcher_config['source']) && is_string($fetcher_config['source']) ? $fetcher_config['source'] : '';

The CSV parser for example did *not* set a link.

I think $result->link was removed in D8 because there was no guarantee that it was set. The change was made in the following commit: https://cgit.drupalcode.org/feeds/commit/?h=8.x-3.x&id=da3b00d0b1817b846...
In that commit you see the following change:

-    $result->title = $channel->getTitle();
-    $result->description = $channel->getDescription();
-    $result->link = $channel->getLink();
+    $result->set('feed_title', $channel->getTitle())
+           ->set('feed_description', $channel->getDescription())
+           ->set('feed_url', $channel->getLink());

Plan of action

I think it would be best if this plugin would allow the user to select a source where the source's base url is coming from. For the syndication parser, it would be logical that it would come from 'feed_url', but for other parsers the base url may be stored on a different source column. This means that just like #2976172: Copy Plugin and #2976180: Rewrite Plugin this issue is postponed on #2971881: Plugins that depend on a tamperable item need to know the available properties.

No Request object

Using a Request object in this plugin makes no sense, I think. As I see it, the plugin is meant to make absolute urls from relative urls provided by the source. For that, the base url from the source needs to be taken. A Request object contains url information from the Drupal site you are importing into. That's the destination, not the source.

@jamesdixon
DOMDocument is a global class. And with the Tamper plugin code, you are in a namespace.

To use global classes in namespaced classes, prepend them with a backslash:

$dom = new \DOMDocument();

I’ve no experience with Lando, but I’m used to that php-xml is installed by default on PHP builds that I install. Php-xml is definitely available for the testbot, else Feeds Extensible Parsers tests wouldn’t pass on the testbot. Feeds Extensible Parsers has a Xpath parser which makes use of DOMDocument. And Feeds tests probably wouldn’t pass either, see feeds/src/Component/XmlParserTrait.php.

On some machines I installed PHP by using Acquia Dev Desktop.
On others I installed PHP using homebrew: brew install php@7.4

I understand from using Docker/Lando it’s easier to switch PHP versions. And that it is easier to switch other components as well, like MySQL.

Good work! Here is my review.

1.   +++ b/src/Plugin/Tamper/AbsoluteUrl.php
     @@ -0,0 +1,226 @@
     +  // :TODO: fix this up, we maybe should be using getBaseUrl instead
     +  // https://api.drupal.org/api/drupal/vendor%21symfony%21http-foundation%21Request.php/function/Request%3A%3AgetBaseUrl/8.2.x
     +  /**
     +   * Holds the Request object we use to grab the url info.
     +   *
     +   * @var \Drupal\Core\Lib\Request
     +   */
     +  protected $request;
     +
     

   I guess that this property can be removed. It doesn't seem to be used anymore.

2.   +++ b/src/Plugin/Tamper/AbsoluteUrl.php
     @@ -0,0 +1,226 @@
     +    $config[self::SETTING_SOURCE] = 'source';
     

   Why does this default to 'source'? I think the default should be NULL.

   I also got the "An illegal choice has been detected." when trying to add a Tamper plugin without selecting a source.

3.   +++ b/src/Plugin/Tamper/AbsoluteUrl.php
     @@ -0,0 +1,226 @@
     +    $form[self::SETTING_SOURCE] = [
     +      '#type' => 'radios',
     +      '#title' => $this->t('Source'),
     +      '#options' => $replace,
     +      '#default_value' => $this->getSetting(self::SETTING_SOURCE),
     +      '#description' => $this->t('The source column name which holds the base URL. If you had a CSV source with a column header of base_url, then you would include base_url here as an example.  The value in the source column should not include a trailing slash. For example: https://someurlofyourchoice.com'),
     +    ];
     

   I think that this setting should be required.

4.   +++ b/src/Plugin/Tamper/AbsoluteUrl.php
     @@ -0,0 +1,226 @@
     +    static $dom;
     ...
     +    if (!isset($dom)) {
     +      $dom = new \DOMDocument();
     +    }
     

   Not sure if this is wrong, but what's the reason of keeping the DOMDocument in memory? The new HTML is loaded sometime later anyway:
   $dom->loadHTML($data);

5.   +++ b/src/Plugin/Tamper/AbsoluteUrl.php
     @@ -0,0 +1,226 @@
     +    // Supress warnings for invalid HTML.
     

   Minor: 'Suppress' is with double 'P'. (I saw the spelling error marked in my editor.)

6.   +++ b/src/Plugin/Tamper/AbsoluteUrl.php
     @@ -0,0 +1,226 @@
     +  /**
     +   * Convert URL to absolute.
     +   */
     +  protected function convertAbsoluteUrl($r_url, &$urls, $b) {
     

   Describe the parameters in the function docblock.

7.   +++ b/src/Plugin/Tamper/AbsoluteUrl.php
     @@ -0,0 +1,226 @@
     +  /**
     +   * Join parts of the URL together.
     +   */
     +  protected function joinUrl($parts) {
     

   Describe the parameter in the function docblock. Also, I assume $parts is always an array, so a typehint "array" could be added to the parameter.

8. If I use 'drupal.org' as base url source, I'll get the following errors:

   Notice: Undefined index: scheme in Drupal\tamper\Plugin\Tamper\AbsoluteUrl->convertAbsoluteUrl() (line 142 of /Users/youri/Sites/devdesktop/drupal8/modules/wip/feeds8/tamper/src/Plugin/Tamper/AbsoluteUrl.php).

   Notice: Undefined index: host in Drupal\tamper\Plugin\Tamper\AbsoluteUrl->convertAbsoluteUrl() (line 149 of /Users/youri/Sites/devdesktop/drupal8/modules/wip/feeds8/tamper/src/Plugin/Tamper/AbsoluteUrl.php).

   Maybe the plugin should automatically add 'http' if it is missing. Good to add test coverage for this case.

9. I get the same errors when leaving base_url empty. It would be good to add a test case for if base_url is empty.
10. If $site_url is an array, for example $site_url = ['https://drupal.org'];, I get the following error:

    Warning: parse_url() expects parameter 1 to be string, array given in Drupal\tamper\Plugin\Tamper\AbsoluteUrl->tamper() (line 83 of /Users/youri/Sites/devdesktop/drupal8/modules/wip/feeds8/tamper/src/Plugin/Tamper/AbsoluteUrl.php).

    If it's an array, I think it would be a good idea to pick the first item of it. And the code should check if the site url is a string. If not, it should abort the operation, I think.

    Needs tests.

11. I think it would be good to add a test case that the plugin is not touching links that are already absolute:
    

    'urls that are already absolute' => [
      '<a href="https://www.example.com/foo">bar</a>',
      '<a href="https://www.example.com/foo">bar</a>',
    ],
    

12. Suggestion: the test method testAllTheThingsAbsolute() could have a third parameter in which you pass the base url. It might be easier this way to add more cases where the base url differs.
13. There are a few coding standards issues. See https://www.drupal.org/pift-ci-job/1880908

+++ b/src/Plugin/Tamper/AbsoluteUrl.php
@@ -0,0 +1,226 @@
+    foreach ($this->sourceDefinition->getList() as $source) {
+      $replace[$source] = $source;
+    }

Just as for the "Copy" plugin, this should display the source's labels, but store the keys:

foreach ($this->sourceDefinition->getList() as $key => $label) {
  $sources[$key] = $label;
}

Change from #3185025: Pass source labels to a Tamper source definition is required to see the difference in the Feeds Tamper UI.

@jamesdixon
Good work! I added a new review. :) I only reviewed the code, I did not try the new code yet.

There are also still code style issues to be fixed as can be seen on https://www.drupal.org/pift-ci-job/1958675.

Gave this a rebase and review - still a bit more work needed

I hope I've addressed all the issues. Will look again later.

I've checked again. I added code to support base urls that do not include the scheme. So previously, "example.com" was marked as incorrect, because parse_url() does not see that as a url that contains a domain name. With the regular expression /^([^\/]+\.[a-z]+)\//i, the code can accept "example.com" as a base url. Tests cases are added for this and it is checked that "www.example.com", "example.com/cat" and "example.com/foo.bar" are correctly parsed as well.

There is one remaining unresolved thread and that is that there is a inconsistency with other Tamper plugins in that it tries to convert the incoming data to a string, where other plugins would throw a TamperException. I'm not sure if that is a problem or if it is worth it to let this issue hang because of that. In my opinion, at least in the context of Feeds, I think it is a good thing if the Tamper plugin tries to be "friendly" and just see if it can use the data. Perhaps the other Tamper plugins need to be changed to become friendlier to or maybe there should exist a strict mode if it is important that the source data is of the correct data type. Changing other Tamper plugins are out of scope for this issue, however.

I think that this is ready to go, although it is possible that a small change is needed after #3332785: Improve handling of empty data is resolved.

Thanks @megachriz

Changes and comments all make sense to me - looks good!

Thanks for reviewing @ericgsmith! I merged the code.