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 api properties and typo #1791

Merged
merged 5 commits into from
Mar 16, 2016
Merged

fix api properties and typo #1791

merged 5 commits into from
Mar 16, 2016

Conversation

tcitworld
Copy link
Member

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Documentation no
Translation no
Fixed tickets #1780, #1784
License MIT

Yeah, because a (bool) string is always true...
Also, cs.

@@ -122,11 +122,11 @@ public function postEntriesAction(Request $request)
$this->get('wallabag_core.content_proxy')->assignTagsToEntry($entry, $tags);
}

if (true === (bool) $isStarred) {
if ($isStarred === 'true') {
Copy link
Member

Choose a reason for hiding this comment

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

What if user send starred=1 or starred=0 ?
And I think we should better use 0/1 instead of true false as a string. It'll a lot easier.

And we should check about retrieving entries to apply the same policy

Copy link
Member Author

Choose a reason for hiding this comment

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

In that case, we should also change values retrieved to 0/1, what do you think ?

Copy link
Member

Choose a reason for hiding this comment

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

Yep

Le 16 mars 2016 à 19:29, Thomas Citharel notifications@github.com a écrit :

In src/Wallabag/ApiBundle/Controller/WallabagRestController.php:

@@ -122,11 +122,11 @@ public function postEntriesAction(Request $request)
$this->get('wallabag_core.content_proxy')->assignTagsToEntry($entry, $tags);
}

  •    if (true === (bool) $isStarred) {
    
  •    if ($isStarred === 'true') {
    
    In that case, we should also change values retrieved to 0/1, what do you think ?


You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub

@tcitworld
Copy link
Member Author

OK, then we'll have to change the docs.

@tcitworld tcitworld force-pushed the v2-fix-api-entry-properties branch 2 times, most recently from 718a168 to dcedb5f Compare March 16, 2016 19:41
'star' => true,
'archive' => false,
'starred' => '1',
'archive' => '0',
Copy link
Member

Choose a reason for hiding this comment

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

Could you add extra tests with value like 'starred' => 1, 'archive' => 0, without single quotes to check the behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

if (true === (bool) $isArchived) {
$entry->setArchived(true);
if (!is_null($isArchived)) {
$entry->setArchived((bool) $isArchived);
Copy link
Member Author

Choose a reason for hiding this comment

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

I put them this way because you can update an entry with starred/archived at false by adding it again (#1797)

Copy link
Member

Choose a reason for hiding this comment

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

👍

j0k3r added a commit that referenced this pull request Mar 16, 2016
@j0k3r j0k3r merged commit 1978d0e into v2 Mar 16, 2016
@j0k3r j0k3r deleted the v2-fix-api-entry-properties branch March 16, 2016 22:23
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