Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yannickoo created an issue. See original summary.

yannickoo’s picture

Status: Needs review » Needs work

The last submitted patch, 2: twitter_block-2783737-username_instead_widgetid-1.patch, failed testing.

yannickoo’s picture

Status: Needs review » Needs work

The last submitted patch, 4: twitter_block-2783737-username_instead_widgetid-4.patch, failed testing.

yannickoo’s picture

Status: Needs work » Needs review
FileSize
6.48 KB

Finally :D

yannickoo’s picture

Okay last try: I removed the description of the username field because it's self-explaining.

naveenvalecha’s picture

Status: Needs review » Needs work
  1. +++ b/config/schema/twitter_block.schema.yml
    @@ -1,17 +1,17 @@
    -    widget_id:
    +    username:
           type: string
    -      label: 'Widget ID'
    +      label: 'Username'
    

    This patch is doing changes in the schema. It needs an hook_post_upate_NAME function to update the existing blocks.

  2. +++ b/src/Plugin/Block/TwitterBlock.php
    @@ -17,36 +17,26 @@ use Drupal\Core\Url;
    -      '#description' => $this->t('Each Twitter Block block requires a unique
    -        widget ID which determines, among other things, the source (user
    -        timeline, favourites, list or search) of the tweets to display. You can
    -        view a list of your existing embedded timeline widgets (and their widget
    -        IDs) or create new embedded timeline widgets by visiting the
    -        <a href="@widgets_section">widgets section of your Twitter settings
    -        page</a> (make sure that you\'re logged in). You can determine a
    -        widget\'s ID by editing it and inspecting the URL (which should be in
    -        the form of twitter.com/settings/widgets/WIDGET_ID/edit) or by looking
    -        at the widget\'s embed code (look for data-widget-id="WIDGET_ID").',
    -        ['@widgets_section' => 'https://twitter.com/settings/widgets']),
    

    +1

  3. +++ b/src/Plugin/Block/TwitterBlock.php
    @@ -175,63 +165,62 @@ class TwitterBlock extends BlockBase {
    -      $render['block']['#attributes']['width'] = $config['width'];
    +      $render['block']['#attributes']['data-width'] = $config['width'];
    

    Unrelated change. Do it in another followup issue.

  4. +++ b/src/Plugin/Block/TwitterBlock.php
    @@ -175,63 +165,62 @@ class TwitterBlock extends BlockBase {
    -      $render['block']['#attributes']['height'] = $config['height'];
    +      $render['block']['#attributes']['data-height'] = $config['height'];
    

    Unrelated change. Do it in another followup issue.

yannickoo’s picture

Status: Needs work » Needs review

Hey :) First of all thank you for reviewing my changes! I guess these changes cannot land in the current branch because there are breaking changes. I mean we cannot transform former widget IDs to usernames, so we need to create a new branch 8.x-3.x. People cannot simply upgrade the module.

Regarding 3 & 4: These changes are necessary to make the block compatible with the latest documentation, I see here no reason to move these 2 dimension attribute changes into a seperate issue.

What do you think? :)

PS: When you agree on 3.x branch you need to push that branch so I can create proper patches for it.

naveenvalecha’s picture

Agreed with the new branch name as its not possible for find the twitter handler from widget id. Created a new branch forked from 8.x-2.x

naveenvalecha’s picture

Version: 8.x-2.x-dev » 8.x-3.x-dev
yannickoo’s picture

I guess we can use the patch from #7 but I have also created a fresh patch base on the new branch :)

pheski’s picture

Here is relevant info from Twitter:

You can at the moment still create search timelines which will result in a widget ID. For the rest, inside the widget settings on your profile page, you'll find that we've moved to a new "no configuration" model, which means that it is possible to use our publish.twitter.com portal to retrieve the simple embed code you need for collections, user timelines, etc. We are also in the process of moving all of our web embed properties to the publish portal, e.g. buttons etc.

The new model means that there are no "widgets" as such any more - so there are no IDs.

In the past couple of months we've rolled out more changes, and it is possible that plugin and other developers should be looking at our oEmbed support for timelines to pull in the data that they were previously picking up the widget ID.

We believe that for the vast majority of use cases and publishers embedding Twitter content, this is a simpler and better experience. We absolutely do hear you if you're using an alternative third-party solution, and we apologise for the disruption as third parties move over to our newer system. We would love to hear from you about the specific plugins or systems you are using that this disrupts, so that (where possible) we can identify and work with developers to make this better.

Pascal-’s picture

Priority: Normal » Critical

patch works for me

Pascal-’s picture

Component: Documentation » Code
naveenvalecha’s picture

Status: Needs review » Fixed

Committed and pushed to 8.x-3.x

There's not any upgrade path from 8.x-2.x to 8.x-3.x

//Naveen

Status: Fixed » Closed (fixed)

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