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

F::load(): guard from output #4656

Merged
merged 6 commits into from
Oct 10, 2022
Merged

Conversation

distantnative
Copy link
Member

@distantnative distantnative commented Sep 17, 2022

This PR …

Enhancement

Ready?

  • Unit tests for fixed bug/feature
  • In-code documentation (wherever needed)
  • Tests and checks all pass

For review team

@distantnative distantnative self-assigned this Sep 17, 2022
@distantnative distantnative added the type: enhancement ✨ Suggests an enhancement; improves Kirby label Sep 17, 2022
@distantnative distantnative removed the request for review from lukasbestle September 17, 2022 13:28
src/Http/Response.php Outdated Show resolved Hide resolved
@distantnative
Copy link
Member Author

distantnative commented Sep 18, 2022

@lukasbestle not sure if this test is actually working or I am testing it wrong. A unit test like this does not fail (while it probably should):

/**
 * @covers ::load
 */
public function testLoadWithInvalidOutput()
{
	F::load(__DIR__ . '/fixtures/f/load/FileWithOutput.php');
}

With a file

// empty line

// empty line
<?php
return [];

@lukasbestle
Copy link
Member

I think headers_sent() doesn't work on the CLI, so it might not be possible to write unit tests for it.

@distantnative
Copy link
Member Author

Hmm I see.
@iskrisis could you please check if this PR solves your issue?

@distantnative distantnative marked this pull request as ready for review September 21, 2022 10:02
@distantnative distantnative added this to the 3.8.0 or 3.8.x milestone Sep 21, 2022
@distantnative distantnative requested a review from a team September 21, 2022 10:02
@distantnative distantnative modified the milestones: 3.8.x, 3.8.1 Oct 7, 2022
@iskrisis
Copy link

iskrisis commented Oct 9, 2022

@distantnative it does seem to work for the problem i eccountered

Copy link
Member

@lukasbestle lukasbestle left a comment

Choose a reason for hiding this comment

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

I have found a way to test this on the CLI by mocking the headers_sent() function. It even showed a minor bug that I missed during visual review.

@distantnative Could you take a final look at the changes and tests?

@lukasbestle lukasbestle linked an issue Oct 9, 2022 that may be closed by this pull request
@distantnative
Copy link
Member Author

Looks good to me!

@distantnative distantnative merged commit ec171a0 into develop Oct 10, 2022
@distantnative distantnative deleted the fix/4391-stray-whitespace branch October 10, 2022 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement ✨ Suggests an enhancement; improves Kirby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Corrupted media on first serve because of stray whitespace
3 participants