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.

Sign up for GitHub

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

Jump to bottom

Use httpLastModified field for If-Modified-Since header on feed updates #2119

Merged
merged 2 commits into from
Mar 23, 2023
Merged

Use httpLastModified field for If-Modified-Since header on feed updates #2119

merged 2 commits into from
Mar 23, 2023

Conversation

rwunderer
Copy link
Contributor

Summary

Hand httpLastModified database field to FeedIo for setting If-Modified-Since header

Checklist

@rwunderer rwunderer mentioned this pull request Feb 25, 2023
Closed
3 tasks
@Grotax Grotax force-pushed the use_http_last_modified_in_fetching branch from 90f5035 to 32bee4d Compare March 1, 2023 19:04
@Grotax
Copy link
Member

Grotax commented Mar 2, 2023

What would be an appropriate date for the first request to create a new feed. If I pass null to feed-io we get this date long ago send.

But if I send the current date that might mean that we get a "ok nothing changed" back and in the end have no items?

I haven't checked yet how it used to be done before..

@rwunderer
Copy link
Contributor Author

What would be an appropriate date for the first request to create a new feed. If I pass null to feed-io we get this date long ago send.

Maybe something like "1 year ago" ?

@Grotax
Copy link
Member

Grotax commented Mar 2, 2023

Yea one year ago could work.

I checked how it used to be, a bit different to how this draft is developing of course but basically null was passed to feedio, and therefore no date and then whatever the default was.

But if we would now use null, we end up with the same situation that feeds would block news for the strange date.

@Grotax Grotax force-pushed the use_http_last_modified_in_fetching branch from 8a9fba7 to 15959e5 Compare March 2, 2023 12:31
@Grotax Grotax force-pushed the use_http_last_modified_in_fetching branch from 15959e5 to 7ed0aff Compare March 20, 2023 11:41
@Grotax Grotax added this to the Improve background job milestone Mar 20, 2023
@Grotax
Copy link
Member

Grotax commented Mar 21, 2023

Signed build 21.2.0-beta1 of this PR
news.tar.gz

@Grotax
Copy link
Member

Grotax commented Mar 21, 2023

I'm currently testing this in my dev instance. And it seems to work fine.

I imported all feeds from my normal prod instance added another one reported as problematic and no feed complained so far. Some favicon errors but that is normal.

First updates also without errors.

@Grotax
Copy link
Member

Grotax commented Mar 21, 2023

Ah and timestamp "one year ago" also send correctly :) tested with webhook.site

@rwunderer
Copy link
Contributor Author

Nice! 👍
I'm giving it a try!

@rwunderer @Grotax
Use httpLastModified field for If-Modified-Since header when fetching…
d5b2f0c 
… feed updates

Signed-off-by: Robert Wunderer <robert.wunderer@caprisys.at>
@Grotax Grotax force-pushed the use_http_last_modified_in_fetching branch from 88c90b9 to 4475d80 Compare March 21, 2023 12:41
@Grotax Grotax mentioned this pull request Mar 21, 2023
Merged
SMillerDev
SMillerDev reviewed Mar 22, 2023
View reviewed changes
lib/Service/FeedServiceV2.php Outdated Show resolved Hide resolved
lib/Service/FeedServiceV2.php Outdated Show resolved Hide resolved
@Grotax @SMillerDev
If timestamp is null use timestamp "one year ago"
2625c8c 
if timestamp is not set during creation of a feed use date one year ago
code fixes and linting fixes.

Co-authored-by: Sean Molenaar <SMillerDev@users.noreply.github.com>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
@Grotax Grotax force-pushed the use_http_last_modified_in_fetching branch from 6917696 to 2625c8c Compare March 22, 2023 11:03
@Grotax
Copy link
Member

Grotax commented Mar 22, 2023

Rebased the code and squashed some commits, I'm happy with the code and would merge it soon.

I want to add some other features before making a new release, but I could create beta releases.

@Grotax Grotax merged commit b1476e9 into nextcloud:master Mar 23, 2023
Grotax added a commit that referenced this pull request Mar 23, 2023
@Grotax
Release 21.2.0-beta1
cf8d9ea 
Changed
- Use httpLastModified field for If-Modified-Since header when fetching feed updates (#2119)

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
@Grotax Grotax mentioned this pull request Mar 23, 2023
Merged
Grotax added a commit that referenced this pull request Mar 24, 2023
@Grotax
Release 21.2.0-beta1
758ea72 
Changed
- Use httpLastModified field for If-Modified-Since header when fetching feed updates (#2119)

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
@Grotax
Copy link
Member

Grotax commented Apr 15, 2023

@SMillerDev I tested with git bisect and it seems that this PR started to break the integration test, the missing GUID is not throwing an exception anymore for some reason. I don't know why we missed that.
But looking at the code I also don't understand how the changes could impact the exception.

@Grotax
Copy link
Member

Grotax commented Apr 15, 2023

Ok just found the cause ...

@Mynacol Mynacol mentioned this pull request May 13, 2023
Closed
@Grotax Grotax mentioned this pull request Jan 17, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SMillerDev SMillerDev SMillerDev approved these changes

Assignees
No one assigned
Labels
3. to review
Projects
None yet
Milestone
Improve background job
Development

Successfully merging this pull request may close these issues.

httpLastModified database field is not used for feed updates
3 participants
@rwunderer @Grotax @SMillerDev