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 #2384: Fix import of WP Recipe Maker recipes #2385

Merged
merged 6 commits into from
Jun 26, 2024

Conversation

nathanielhourt
Copy link

  • Fix recipe import from some WordPress sites
  • Add test exercising new parser logic

Topic and Scope

Many recipe websites are built with WordPress Recipe Maker with Yoast SEO exposing the recipe as a parseable
schema. This combination presented a schema which the recipe extractor did not handle properly.

This PReq makes a small modification to the parser logic to support this new schema format, and adds a test
exercising the new functionality.

I have found several sites that previously failed to parse, and now parse successfully. I believe this update
resolves the issue with most WordPress Recipe Maker websites, and it definitely resolves the parsing failure
on the site referenced in #2384.

Concerns/issues

The tests do not detect any regressions from this change, and I do not expect the change to be a significant
risk of causing regressions.

Formal requirements

There are some formal requirements that should be satisfied. Please mark those by checking the corresponding box.

  • I did check that the app can still be opened and does not throw any browser logs
  • I created tests for newly added PHP code (check this if no PHP changes were made)
  • I updated the OpenAPI specs and added an entry to the API changelog (check if API was not modified)
  • I notified the matrix channel if I introduced an API change

Nathaniel added 2 commits June 23, 2024 12:08
I want to import recipes from a site that uses WordPress Recipe Maker

At least on this site, the schema is structured like so:

{
  "@Schema": "https://schema.org/",
  "@graph": [
    { /* blah */ },
    { /* blah */ },
    { "@type": "Recipe", "otherStuff": True }
  ]
}

Notably missing is the @Schema on the Recipe object. It's intended
to be inherited from the parent, but HttpJsonLdParser didn't support
this kind of inheritance.

Well, now it does! =)

Signed-off-by: Nathaniel <I@nathaniel.land>
Following the update to the parser to support inheriting the schema
declaration from the root object to the recipe object, add a test
that exercises this new functionality.

Signed-off-by: Nathaniel <I@nathaniel.land>
Copy link

github-actions bot commented Jun 24, 2024

Test Results

   12 files    584 suites   1m 35s ⏱️
  575 tests   575 ✅ 0 💤 0 ❌
2 300 runs  2 299 ✅ 1 💤 0 ❌

Results for commit fc29da7.

♻️ This comment has been updated with latest results.

Signed-off-by: Christian Wolf <github@christianwolf.email>
Signed-off-by: Christian Wolf <github@christianwolf.email>
Reported by psalm upon pushing

Signed-off-by: Christian Wolf <github@christianwolf.email>
Copy link
Collaborator

@christianlupus christianlupus left a comment

Choose a reason for hiding this comment

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

Looks good. Hopefully this will not break any other pages but we can fix that later on.

Signed-off-by: Christian Wolf <github@christianwolf.email>
@christianlupus christianlupus merged commit 26cfe81 into nextcloud:master Jun 26, 2024
18 checks passed
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.

2 participants