Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a failure in the URL of News Feeds if the Feed-Provider sends Spaces in the Link to the Feed #9033

Closed
wants to merge 3 commits into from

Conversation

astridx
Copy link
Contributor

@astridx astridx commented Jan 30, 2016

  1. Go to Components|News Feeds|Feeds and create a new News Feed with the Link to
    https://www.datev.de/web/de/rss/nachrichten-steuern-und-recht.rss and call it Datev
    datev2
  2. Create a Menu Item of the type Single News Feed and select the Feed Datev.
    datev1
  3. Open this Menu Item in the Frontend and see, that die Links to the Feeds are not correct.
    datev3

The reason is this: DATEV adds a Space to the URI and because of this in Joomla the address is displayed not correct.
Apply my patch and see that die Links to the Feed are correct now.

Update (2016-02-04):
4. Drop my patch and go to Extensions|Modules and create a new Module of the type Feed Display with the Link to
https://www.datev.de/web/de/rss/nachrichten-steuern-und-recht.rss and call it Test. Apply it to position debug and show it on all pages.
feed

  1. Go to the front page and see, that there are no links shown in the Module.
  2. Apply my patch and see in the front end, that no the links are correct!
    feed2

…the Link to

https://www.datev.de/web/de/rss/nachrichten-steuern-und-recht.rss and call it Datev
2. Create a Menu Item of the type Single News Feed and select the Feed Datev.
3. Open this Menu Item in the Frontend and see, that die Links to the Feeds are not correct. The reason is this: DATEV add a Space to the URI and because of this in Joomla the address is displayed not correct.
Apply my patch and see that die Links to the Feed now correct.
@wilsonge
Copy link
Contributor

Please remove the nbproject file

@richard67
Copy link
Member

I have tested this item ✅ successfully on 50634cc

Tested as described with success.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9033.

@cyrezdev
Copy link
Contributor

cyrezdev commented Feb 3, 2016

I was able to reproduce the issue (due to a leading "space" added to uri from JFeedFactory...)

But i wonder if the code could not be cleaned in the same time ?
What is supposed to be used for the $uri variable : https://github.com/astridx/joomla-cms/blob/Newsfeed/components/com_newsfeeds/views/newsfeed/tmpl/default.php#L108-L109
Is this $uri used nowhere in the file is needed ? (i wonder...)

Why not something like this :

    <!-- Show items -->
    <?php if (!empty($this->rssDoc[0])) { ?>
        <ol>
        <?php for ($i = 0; $i < $this->item->numarticles; $i++) { ?>
            <?php if (empty($this->rssDoc[$i])) { break; } ?>
            <?php
                $uri    = trim($this->rssDoc[$i]->uri);
                $title  = trim($this->rssDoc[$i]->title);
                $text   = !empty($this->rssDoc[$i]->content) || !is_null($this->rssDoc[$i]->content) ? $this->rssDoc[$i]->content : $this->rssDoc[$i]->description;
            ?>
            <li>
            <?php if (!empty($this->rssDoc[$i]->uri)) : ?>
                <a href="<?php echo $uri; ?>" target="_blank"><?php echo $title; ?></a>
            <?php else : ?>
                <h3><?php  echo '<a target="_blank" href="' . $uri . '">' . $title . '</a>'; ?></h3>
            <?php  endif; ?>
            <?php if ($this->params->get('show_item_description') && !empty($text)) : ?>
                <div class="feed-item-description">
                <?php if ($this->params->get('show_feed_image', 0) == 0)
                {
                    $text = JFilterOutput::stripImages($text);
                }
                $text = JHtml::_('string.truncate', $text, $this->params->get('feed_character_count'));
                echo str_replace('&apos;', "'", $text);
                ?>
                </div>
            <?php endif; ?>
            </li>
        <?php } ?>
        </ol>
    <?php } ?>

Note: tabs not repecting J code standards in this file... Maybe a clean of the code could be welcomed in the same time ;-)

fixed the same failure in mod_feed.
@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @richard67


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9033.

@richard67
Copy link
Member

I have tested this item ✅ successfully on bfb2c2d

Tested with success both menu item and module.
Test for menu item as described.
Test for module: Before patch, item titles are just text. After patch, item titles are links with correct URLs.
@astridx Could you update test instructions by the module for other testers?
@JoomliC Could you test this, too?


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9033.

@astridx
Copy link
Contributor Author

astridx commented Feb 4, 2016

@richard67
Thank you for testing. I will update the instructions later.

@cyrezdev
Copy link
Contributor

cyrezdev commented Feb 6, 2016

I will try to test it on Sunday ;-)
Just a few more comments on styling (this file was not well formatted before your PR @astridx , but could be nice in the same PR to clean all as you already started to! ;-) ) :

  • Some <?php echo with 2 spaces between php & echo could be replaced by only one
  • Just a "not-related" styling tab issue that "hurts" my vision of the file :
    <!-- Show Image -->
    <?php if (isset($this->rssDoc->image) && isset($this->rssDoc->imagetitle) && $this->params->get('show_feed_image')) : ?>
    <div>
        <img src="<?php echo $this->rssDoc->image; ?>" alt="<?php echo $this->rssDoc->image->decription; ?>" />
    </div>
    <?php endif; ?>

;-)

@andrepereiradasilva
Copy link
Contributor

one question i have. Shouldn't that "trim" be made in the feed parser library so that it applies to all feeds read by joomla no matter what is the usage context?

IIRC it would be done in:

@grhcj
Copy link

grhcj commented Feb 23, 2016

I have tested this item ✅ successfully on bfb2c2d


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9033.

@brianteeman
Copy link
Contributor

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9033.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 10, 2016
@wilsonge wilsonge added this to the Joomla 3.6.0 milestone Jun 15, 2016
@s-sential
Copy link

s-sential commented Jun 24, 2016

Hi, I'm new to GitHub and I found it as I have the mentioned problem with the Datev RSS Feed. I have already installed the feed, but I do not find your "patch" to solve the link problem. Sorry for my neebie unknowing ;o) Is it a file which I can download? Thank you very much in advance, Sabrina

@grhcj
Copy link

grhcj commented Jun 24, 2016

Hello @s-sential please read https://docs.joomla.org/Testing_Joomla!_patches to find out how to work with issues. Additionally, the patch will be soon in the normal joomla code (which you can download on the joomla website).

roland-d pushed a commit to roland-d/joomla-cms that referenced this pull request Jun 25, 2016
@roland-d
Copy link
Contributor

roland-d commented Jun 25, 2016

Merged with commit 0048ca9 Thanks everybody.

@roland-d roland-d closed this Jun 25, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 25, 2016
@s-sential
Copy link

@grhcj Thank you very much for your support. I have successfully installed the Patch-Tester in Joomla but unfortunately I cannot find the patch in the offered list (in Joomla Backend). Is the patch still availbale there? Thank you very much in advance and sorry again for my newbie questions

@brianteeman
Copy link
Contributor

brianteeman commented Jun 25, 2016 via email

@astridx
Copy link
Contributor Author

astridx commented Jun 25, 2016

@s-sential
As Brian wrote this patch is not available any more within the patch tester component, because it is merged and it will be integrated in one of the next versions of Joomla!.
I am not sure if it will be available in Joomla 3.6. In this case you can only have to wait a few days. Joomla! 3.6 is scheduled for Tuesday: https://www.joomla.org/announcements/release-news/5661-joomla-3-6-beta-2-released.html
Do you use the newsfeed module or the component? Do you need it urgent? Basically you need to change only one line in the Joomla code and you can do this with an override. If you use the component, you can find this line on this page: 4d9a3b1

@astridx
Copy link
Contributor Author

astridx commented Jun 25, 2016

I see this message: This pull request is closed, but the astridx:Newsfeed branch has unmerged commits. But I do not see witch commit is unmerged?
Should I do anything else?

@grhcj
Copy link

grhcj commented Jun 26, 2016

Hello,
@astridx The maintainers merged this with the new commit 0048ca9 (see comment above). So this PR is marked as "closed", but the changes are in the staging branch.
@s-sential The changes will be in the 3.6 release because they are in the staging/master branch (except if there are some big problems with that and they are reverted).

@astridx
Copy link
Contributor Author

astridx commented Jun 26, 2016

@ grhcj
OK, thank you. Now I understood.

@astridx astridx deleted the Newsfeed branch September 18, 2016 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet