Fix Missing Publishing Context HTTP Error#814
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where HTTP error details were not being passed to ErrorPages during publishing. Key changes include:
- Enhancing TestErrorPage to accept an errorChecker closure for HTTP errors.
- Updating the Site tests to verify the proper HTTP error is passed.
- Refactoring PublishingContext-Rendering and EnvironmentValues to pass HTTPError explicitly.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Tests/IgniteTesting/TestWebsitePackage/Sources/TestErrorPage.swift | Adds an initializer and errorChecker closure to TestErrorPage to verify HTTPError propagation. |
| Tests/IgniteTesting/Publishing/Site.swift | Updates tests to compare the expected error status with the one passed to TestErrorPage. |
| Sources/Ignite/Publishing/PublishingContext-Rendering.swift | Removes direct environment assignment and passes HTTPError via the initializer. |
| Sources/Ignite/Framework/Environment/EnvironmentValues.swift | Updates initializers to accept and store a HTTPError instance. |
Comments suppressed due to low confidence (1)
Sources/Ignite/Publishing/PublishingContext-Rendering.swift:153
- Verify that passing HTTPError via the initializer fully replaces the previous behavior of setting environment.httpError directly, ensuring that all downstream consumers receive the correct error instance.
httpError: error
JPToroDev
left a comment
JPToroDev
left a comment
There was a problem hiding this comment.
Great catch! Couple small notes, otherwise looks great!
| allContent: [Article], | ||
| pageMetadata: PageMetadata, | ||
| pageContent: any LayoutContent | ||
| pageContent: any LayoutContent, |
There was a problem hiding this comment.
Can we create a dedicated init that mirrors the other convenience initializers?
There was a problem hiding this comment.
Not sure what you mean by this? EnvironmentValues has 5 different inits and I only modified the one that required the fix
| let expectedError = PageNotFoundError() | ||
|
|
||
| let errorPage = TestErrorPage(title: "A different title", description: "A different description") { error in | ||
| #expect(error.statusCode == expectedError.statusCode) |
There was a problem hiding this comment.
Could we simply check #expect(errorPage.error.statusCode == expectedError.statusCode) instead of introducing the errorChecker closure?
There was a problem hiding this comment.
Heya sorry for the delay, life happened.
Unless I'm missing something, checking the page's error after calling site.publish() will not work as it's set (and reset) dynamically during publishing?
What I want the test to do is to guarantee that the correct error is present during site publishing. I can only do so by injecting this check when the rendering actually happens.
There was a problem hiding this comment.
Gotcha—thanks for clarifying!
MrSkwiggs
force-pushed
the
bugfix/publishing-context-http-error
branch
from
d15ddaa to
2df9f80
Compare
MrSkwiggs
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the missing publishing context HTTP error by ensuring that HTTP errors are correctly passed to ErrorPages and updating tests accordingly.
- Updated TestErrorPage to include an errorChecker closure that validates the HTTPError.
- Modified the Site test to assert against the expected HTTPError.
- Adjusted PublishingContext and EnvironmentValues initializers to propagate the HTTPError to downstream components.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Tests/IgniteTesting/TestWebsitePackage/Sources/TestErrorPage.swift | Added an errorChecker parameter and updated the body to invoke it. |
| Tests/IgniteTesting/Publishing/Site.swift | Updated test to verify the HTTPError via errorChecker. |
| Sources/Ignite/Publishing/PublishingContext-Rendering.swift | Removed direct assignment of environment.httpError and passed httpError to the PageMetadata initializer. |
| Sources/Ignite/Framework/Environment/EnvironmentValues.swift | Added httpError parameter to the initializer and set it in EnvironmentValues. |
Comments suppressed due to low confidence (1)
Sources/Ignite/Publishing/PublishingContext-Rendering.swift:152
- [nitpick] Since the HTTP error is now passed explicitly via the initializer, please confirm that downstream components utilize the provided 'httpError' and do not depend on a global assignment.
pageContent: site.errorPage,
We actually weren’t passing the relevant HTTP errors to
ErrorPages (either we never did, or it broke at some point without anyone noticing). The existing tests only checked for static strings, so they didn’t catch that the actual error instances weren’t being passed around correctly.That’s now fixed, and the tests have been updated to properly check for this going forward.