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: do not fail on invalid time strings #3028

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Jan 5, 2025

Fixes the updater job to fail with:

    "Exception": "DateMalformedStringException",
    "Message": "Failed to parse time string (GgZzIMMLoE/7N24qteM295g8pA) at position 0 (G): The timezone could not be found in the database",

I had this for a long while until I actually looked into it. And suddenly I get articles from other feeds again 😅 I suppose I have (had?) one feed that provides invalid data, causing this issue.

DateMalformedStringException was introduced with PHP 8.3 only so to keep compatibility with 8.2 the catch goes against the general \Exception.

If intertested, unfold to see my stack trace.
{
  "reqId": "4CoKlJOL9g3G2CEHgaud",
  "level": 3,
  "time": "2024-12-15T18:20:34+00:00",
  "remoteAddr": "",
  "user": "--",
  "app": "core",
  "method": "",
  "url": "--",
  "message": "Error while running background job OCA\\News\\Cron\\UpdaterJob (id: 5806, arguments: null)",
  "userAgent": "--",
  "version": "30.0.2.2",
  "exception": {
    "Exception": "DateMalformedStringException",
    "Message": "Failed to parse time string (GgZzIMMLoE/7N24qteM295g8pA) at position 0 (G): The timezone could not be found in the database",
    "Code": 0,
    "Trace": [
      {
        "file": "/path/to/nextcloud/apps/news/lib/Fetcher/FeedFetcher.php",
        "line": 133,
        "function": "__construct",
        "class": "DateTime",
        "type": "->"
      },
      {
        "file": "/path/to/nextcloud/apps/news/lib/Service/FeedServiceV2.php",
        "line": 284,
        "function": "fetch",
        "class": "OCA\\News\\Fetcher\\FeedFetcher",
        "type": "->"
      },
      {
        "file": "/path/to/nextcloud/apps/news/lib/Service/FeedServiceV2.php",
        "line": 376,
        "function": "fetch",
        "class": "OCA\\News\\Service\\FeedServiceV2",
        "type": "->"
      },
      {
        "file": "/path/to/nextcloud/apps/news/lib/Service/UpdaterService.php",
        "line": 63,
        "function": "fetchAll",
        "class": "OCA\\News\\Service\\FeedServiceV2",
        "type": "->"
      },
      {
        "file": "/path/to/nextcloud/apps/news/lib/Cron/UpdaterJob.php",
        "line": 58,
        "function": "update",
        "class": "OCA\\News\\Service\\UpdaterService",
        "type": "->",
        "args": [
          "*** sensitive parameters replaced ***"
        ]
      },
      {
        "file": "/path/to/nextcloud/lib/public/BackgroundJob/Job.php",
        "line": 61,
        "function": "run",
        "class": "OCA\\News\\Cron\\UpdaterJob",
        "type": "->"
      },
      {
        "file": "/path/to/nextcloud/lib/public/BackgroundJob/TimedJob.php",
        "line": 83,
        "function": "start",
        "class": "OCP\\BackgroundJob\\Job",
        "type": "->"
      },
      {
        "file": "/path/to/nextcloud/lib/public/BackgroundJob/TimedJob.php",
        "line": 73,
        "function": "start",
        "class": "OCP\\BackgroundJob\\TimedJob",
        "type": "->"
      },
      {
        "file": "/path/to/nextcloud/cron.php",
        "line": 162,
        "function": "execute",
        "class": "OCP\\BackgroundJob\\TimedJob",
        "type": "->"
      }
    ],
    "File": "/path/to/nextcloud/apps/news/lib/Fetcher/FeedFetcher.php",
    "Line": 133,
    "message": "Error while running background job OCA\\News\\Cron\\UpdaterJob (id: 5806, arguments: null)",
    "exception": {},
    "CustomMessage": "Error while running background job OCA\\News\\Cron\\UpdaterJob (id: 5806, arguments: null)"
  }
}

@blizzz blizzz requested review from devlinjunker and Grotax January 5, 2025 19:41
@blizzz blizzz force-pushed the fix/noid/handle-DateMalformedStringException branch from f608cb3 to 7afc3ba Compare January 5, 2025 19:44
Copy link
Contributor

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

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

I'm not sure if we want to pretend the broken feeds are actually valid.

Comment on lines +306 to +308
} else {
$pubDT = $lastModified;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this a duplicate with the catch?

Copy link
Member Author

Choose a reason for hiding this comment

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

Aye, $pubDT = $lastModified; could be moved after line 300, then we do not need the else branch and can have the catch block empty.

@blizzz
Copy link
Member Author

blizzz commented Jan 6, 2025

I'm not sure if we want to pretend the broken feeds are actually valid.

Right now it is a hard failure and the whole update process crashes. Not following feeds are updated. So this has to be caught, and with my code changes it would just continue as if $httpLastNotified was null (default) or pubDate or published not set. So, it just falls back to normal behaviour.

@Grotax
Copy link
Member

Grotax commented Jan 9, 2025

@SMillerDev do you still want to refactor the code a bit or should we merge it?

@Grotax Grotax force-pushed the fix/noid/handle-DateMalformedStringException branch from 7afc3ba to 5ab3f0a Compare January 9, 2025 13:09
@Grotax Grotax enabled auto-merge (rebase) January 9, 2025 13:10
@Grotax Grotax merged commit 3361c60 into master Jan 9, 2025
24 checks passed
@Grotax Grotax deleted the fix/noid/handle-DateMalformedStringException branch January 9, 2025 13:12
Grotax added a commit that referenced this pull request Jan 10, 2025
Changed
- add explanations for the individual values in the feed information table (#3031)
- show error message from `opml` import in web-ui (#3036)
- disable new setting "nextUpdateTime" introduced in #2999 by default (#3039)

Fixed
- fix proxy port removed if standard port for the protocol (#3027)
- background updater may stumble over invalid datetime strings from feeds (#3028)

Signed-off-by: Benjamin Brahmer <[email protected]>
@Grotax Grotax mentioned this pull request Jan 10, 2025
Grotax added a commit that referenced this pull request Jan 10, 2025
Changed
- add explanations for the individual values in the feed information table (#3031)
- show error message from `opml` import in web-ui (#3036)
- disable new setting "nextUpdateTime" introduced in #2999 by default (#3039)

Fixed
- fix proxy port removed if standard port for the protocol (#3027)
- background updater may stumble over invalid datetime strings from feeds (#3028)

Signed-off-by: Benjamin Brahmer <[email protected]>
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.

3 participants