Thanks for the module, great to see a stable D8 version. I really like the option to link the thumbnail to the provider URL, as it helps me work around cookie policy issues. However I would appreciate an option to make the URL open in a new tab. Patch will follow.

CommentFileSizeAuthor
#16 interdiff.txt783 bytesgambry
#16 open_link_in_new_tab-2857350-16.patch7.11 KBgambry
#13 open_link_in_new_tab-2857350-13.patch6.3 KBgambry
#5 interdiff-2857350-2-5.txt1.83 KBmarcvangend
#5 2857350-5-open-link-in-new-tab.patch6.32 KBmarcvangend
#2 2857350-2-open-link-in-new-tab.patch4.41 KBmarcvangend
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcvangend created an issue. See original summary.

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
CreditAttribution: marcvangend as a volunteer and at Triquanta commented
Assigned: marcvangend » Unassigned
Status: Active » Needs review
FileSize
2857350-2-open-link-in-new-tab.patch4.41 KB

Here's my patch!

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Needs some tests. Adding a test case to FieldOutputTest is probably good enough here.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented

Yikes, why is the bot saying "No tests found".

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
CreditAttribution: marcvangend as a volunteer and at Triquanta commented
FileSize
2857350-5-open-link-in-new-tab.patch6.32 KB
interdiff-2857350-2-5.txt1.83 KB

Thanks for the quick feedback. I added a test case, and updated a couple of similar test cases.

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
CreditAttribution: marcvangend as a volunteer and at Triquanta commented
Status: Needs work » Needs review
marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
CreditAttribution: marcvangend as a volunteer and at Triquanta commented

@Sam152 documentation says:

Tests found when calling PHPUnit directly, but no tests found by drupal.org's testbot

The Drupal test runner (core/scripts/run-tests.sh) discovers tests in a different way than running phpunit directly. Confirm that your test conforms to the Drupal test runner standard as documented in PHPUnit file structure, namespace, and required metadata: Contributed Modules.

Most likely the Drupal test runner is not able to find your class in the autoloader, which means that the namespace and directory do not conform to the PSR-4 standard.

(https://www.drupal.org/docs/8/phpunit/running-phpunit-tests)

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented

They have been running in the past: https://www.drupal.org/node/1243930/qa

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
CreditAttribution: marcvangend as a volunteer and at Triquanta commented

Sure, I didn't mean to question if they did work before. But it might be part of the answer why they don't work now. When I search Google for "D8.4 No tests found" (including the quotes) I get 137 results and the majority of results is very recent. By contrast, "D8.3 No tests found" yields no results at all.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented

Yeah, it's just odd. I'll rerun them and see if it was just a random testbot fail.

gambry’s picture

gambry
Primary language Italian
Location Milan
CreditAttribution: gambry at manifesto commented

This is a nice piece of functionality, I'm assuming required for 99% of the cases when the link has to point to the provider (and UX best practice suggests to open this links in new tabs).

Trying to re-test as I can see HEAD is now running the tests properly.

If all are green I will remove the tag and RTBC.

Status: Needs review » Needs work

The last submitted patch, 5: 2857350-5-open-link-in-new-tab.patch, failed testing. View results

gambry’s picture

gambry
Primary language Italian
Location Milan
CreditAttribution: gambry at manifesto commented
Status: Needs work » Needs review
FileSize
open_link_in_new_tab-2857350-13.patch6.3 KB

Let's re-roll #5. I haven't changed the patch, just manually updated the bottom of the first hunk from:

/**
   * {@inheritdoc}
   */
  public function viewElements(FieldItemListInterface $items, $langcode) {

to

/**
   * Constructs a new instance of the plugin.
   *
   * @param string $plugin_id

For the patch to apply nicely.

gambry’s picture

gambry
Primary language Italian
Location Milan
CreditAttribution: gambry at manifesto commented

This looks to be the reason for "No tests found":

Fatal error: Declaration of Drupal\video_embed_field_mock_provider\Plugin\video_embed_field\Provider\MockProvider::renderThumbnail($image_style, $link_url) must be compatible with Drupal\video_embed_field\ProviderPluginInterface::renderThumbnail($image_style, $link_url, $link_options = Array) in /var/www/html/modules/contrib/video_embed_field/tests/modules/video_embed_field_mock_provider/src/Plugin/video_embed_field/Provider/MockProvider.php on line 15
16:27:39 PHP Fatal error: Declaration of Drupal\video_embed_field_mock_provider\Plugin\video_embed_field\Provider\MockProvider::renderThumbnail($image_style, $link_url) must be compatible with Drupal\video_embed_field\ProviderPluginInterface::renderThumbnail($image_style, $link_url, $link_options = Array) in /var/www/html/modules/contrib/video_embed_field/tests/modules/video_embed_field_mock_provider/src/Plugin/video_embed_field/Provider/MockProvider.php on line 15

gambry’s picture

gambry
Primary language Italian
Location Milan
CreditAttribution: gambry at manifesto commented
Status: Needs review » Needs work

Working on this.

gambry’s picture

gambry
Primary language Italian
Location Milan
CreditAttribution: gambry at manifesto commented
Status: Needs work » Needs review
FileSize
open_link_in_new_tab-2857350-16.patch7.11 KB
interdiff.txt783 bytes

Status: Needs review » Needs work

The last submitted patch, 16: open_link_in_new_tab-2857350-16.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Status: Needs work » Closed (won't fix)

With the advent of Media in core, the Video Embed Field module has moved to being minimally maintained. Only issues which assist in the migration to Media in core will be committed. To read more about this decision, please see: #3089599: Maintenance status for Video Embed Field now that media is in core.