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

implement bookmarklet #1471

Merged
merged 1 commit into from
Oct 8, 2015
Merged

implement bookmarklet #1471

merged 1 commit into from
Oct 8, 2015

Conversation

nicosomb
Copy link
Member

@nicosomb nicosomb commented Oct 7, 2015

Do we need a test for this PR?

{
$entry = new Entry($this->getUser());
$entry->setUrl($request->get('url'));
$this->updateEntry($entry);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simplify to :

$this->updateEntry(new Entry($this->getUser()), $request->get('url'));

Hum, no sorry..

@nicosomb
Copy link
Member Author

nicosomb commented Oct 8, 2015

Route changed & test added.

public function addEntryViaBookmarklet(Request $request)
{
$url = $request->get('url');
if (filter_var($url, FILTER_VALIDATE_URL)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious, what happend if you don't do that? You got an exception or sth?
This kind of verification is already done in graby.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an exception launched by graby. I have to catch it in fact.

@nicosomb
Copy link
Member Author

nicosomb commented Oct 8, 2015

I removed the test about "Bad URL". In fact, the behaviour if we have a bad URL is the same as before: we create a new entry with no content and "fakeURL" in the URL field.
I think it has to be fixed in graby maybe.

@nicosomb
Copy link
Member Author

nicosomb commented Oct 8, 2015

Tests are green but GitHub lost the relation with travis https://travis-ci.org/wallabag/wallabag

@j0k3r
Copy link
Member

j0k3r commented Oct 8, 2015

Maybe there is something to fix in graby but ContentProxy also need to handle Exception in case of graby fail.
We'll do that later

@@ -5,6 +5,7 @@
use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route;
use Symfony\Bundle\FrameworkBundle\Controller\Controller;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Security\Acl\Exception\Exception;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it used somewhere?
(php-cs-fixer is your friend)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for your vigilence.

@nicosomb
Copy link
Member Author

nicosomb commented Oct 8, 2015

ready to be rebased.

@j0k3r
Copy link
Member

j0k3r commented Oct 8, 2015

Good to be rebased and then merged 👍

j0k3r added a commit that referenced this pull request Oct 8, 2015
@j0k3r j0k3r merged commit 9dbcf9d into v2 Oct 8, 2015
@j0k3r j0k3r deleted the v2-bookmarklet branch October 8, 2015 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants