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

Add updatedDate to item model #43

Merged
merged 6 commits into from
Oct 1, 2016
Merged

Conversation

schaal
Copy link
Contributor

@schaal schaal commented Oct 1, 2016

Implement your suggestions from owncloud-archive/news#982 (comment)

  • update picoFeed to 0.1.25
  • add updated_date to the database
  • use the new getPublishedDate and getUpdatedDate from picoFeed to set the corresponding item values
  • adjust the unit tests

@BernhardPosselt
Copy link
Member

Looks good, did you test it? You need to bump the version to 9.0.5 to start the migration

@schaal
Copy link
Contributor Author

schaal commented Oct 1, 2016

I did a quick test with the web interface and OCReader, should I update this pull request with a bumped version?

@BernhardPosselt
Copy link
Member

yes

@BernhardPosselt
Copy link
Member

BTW: https://github.com/nextcloud/news/blob/master/lib/Service/FeedService.php#L248

This probably needs to be adjusted

@BernhardPosselt
Copy link
Member

👍

@BernhardPosselt BernhardPosselt merged commit e45511f into nextcloud:master Oct 1, 2016
@schaal schaal deleted the pubdate branch October 1, 2016 07:06
@BernhardPosselt
Copy link
Member

Ok, things are broken because getPublishedDate() can return null: PHP Fatal error: Call to a member function getTimestamp() on null in /var/www/html/owncloud/apps/news/lib/Fetcher/FeedFetcher.php on line 243

Will revert PR

@schaal
Copy link
Contributor Author

schaal commented Oct 2, 2016

Why not just use getDate() if getPublishedDate() is null? If this is acceptable to you, I can send another pull request.

@BernhardPosselt
Copy link
Member

@schaal shouldnt that be done in picoFeed instead?

@schaal
Copy link
Contributor Author

schaal commented Oct 2, 2016

Not sure, I could see it either way

@BernhardPosselt
Copy link
Member

Right, you can add the checks at both places but if there's a sensible default it should be done once in the library rather than forcing all users to deal with nulls :D

My impression was that your? pull request to picoFeed already did that or am I wrong?

@schaal
Copy link
Contributor Author

schaal commented Oct 3, 2016

It wasn't my pull request, but looking at the code, it just sets publishedDate and updatedDate to whatever is in the feed, so if they're not in the feed, they are set to null.

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

2 participants