Skip to content

Conversation

@thoresuenert
Copy link
Contributor

@thoresuenert thoresuenert commented Mar 20, 2025

Fixes: #1049

@thoresuenert thoresuenert marked this pull request as draft March 21, 2025 08:42
@thoresuenert thoresuenert marked this pull request as ready for review March 21, 2025 13:49
@brendt
Copy link
Member

brendt commented Mar 22, 2025

The failing tests aren't related to this PR, I'll fix them before merging.

One thing I was wondering about: how to set the referer header with eg. a normal GET request? I believe Laravel stores the previous URL in a session to cover more cases. I could be wrong though. Not saying we need it, just wanted to check

@brendt brendt added this to the v1.0.0-beta.1 milestone Mar 22, 2025
@thoresuenert
Copy link
Contributor Author

I will look into it. it will take some time because its the first time doing something with temptest.
Should i convert the pr to draft until i am finished?

@innocenzi innocenzi marked this pull request as draft March 24, 2025 10:18
@thoresuenert
Copy link
Contributor Author

I looked into the implementation of laravel:

  1. The session knows the previous url/uri. (_previous.url)
  2. In the global session StartSession the value is set to $request->fullUrl()
  3. The Session Store has a contract with set and get previous url/uri.

Do we have a global Middleware which i can use?
I am not sure where to put that logic.

Some ideas, which feels wrong:

  1. I can mess with the HttpApplication run function
    2.I can mess with the Router

@brendt
Copy link
Member

brendt commented Apr 1, 2025

Currently global HTTP middleware is added here: https://github.com/tempestphp/tempest-framework/blob/main/src/Tempest/Router/src/RouterInitializer.php#L19

This will be refactored soon, but you can add it over there for now.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 14191311957

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 80.975%

Totals Coverage Status
Change from base Build 14161923512: 0.01%
Covered Lines: 11066
Relevant Lines: 13666

💛 - Coveralls

@brendt
Copy link
Member

brendt commented Apr 2, 2025

Middleware has now been refactored. The difference is that middleware is discovered, so you won't need to add it anywhere anymore. Just make sure to add a priority like with this one: https://github.com/tempestphp/tempest-framework/blob/main/src/Tempest/Router/src/Cookie/SetCookieMiddleware.php#L13

@thoresuenert
Copy link
Contributor Author

thoresuenert commented Apr 6, 2025

I introduced a new middleware, only for the purpose of storing the previous url in the session.
Finally i found the tests for the other responses in Tests\Tempest\Integration\Http\Responses i was looking for Router\Responses something.

I borrowed two functions from laravel getPreviousUrl and setPreviousUrl and added these to the Session Class.
Later on i figured out that the Request has a function getSessionValue.

It is possible to remove the new functions from the session and use the new const PREVIOUS_URL in combination with the set and get functions directly.

Let me know if i should refactor getPreviousUrl and setPreviousUrl.

PS: I did my best to figure out the right way to test. It is way harder then expected to contribute to the core of a framework ;) Thanks for your patience and help.

@brendt brendt marked this pull request as ready for review April 7, 2025 07:28
@brendt
Copy link
Member

brendt commented Apr 7, 2025

Looking great! Thanks for sticking with it and figuring it out, I actually have nothing to add :)

I'll let CI run and then merge it!

Are you up for PRing the docs as well? A simple entry in this list should be fine: https://github.com/tempestphp/tempest-docs/blob/main/src/Web/Documentation/content/main/1-essentials/02-controllers.md?plain=1#L363

@brendt brendt merged commit 8d43ce5 into tempestphp:main Apr 7, 2025
65 checks passed
@thoresuenert
Copy link
Contributor Author

thoresuenert commented Apr 8, 2025

@brendt of course. tempestphp/tempest-docs#71

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.

Add Back response

3 participants