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

Add Http Foundation to Known Core #3262

Open
wants to merge 19 commits into
base: dev
Choose a base branch
from
Open

Conversation

ipranjal
Copy link
Contributor

@ipranjal ipranjal commented Sep 22, 2024

Here's what I fixed or added:

  • Adds HttpFoundation Request/Response to Known Core
  • Integrate HttpFoundation Session to core
  • Remove all use of super globals

Here's why I did it:

So that non can be compatible with non apache environment like Swoole etc , and also integrate SymphonyResponse compatible package (like required for ActivityPub Integration)

Checklist: ([x] to check/tick the boxes)

  • This pull request addresses a single issue
  • If this code includes interface changes, I've included screenshots in this Pull Request thread
  • I've adhered to Known's style guide (these codesniffer rules might help!)
  • My git branch is named in a descriptive way - i.e., yourname-summary-of-issue
  • I've tested my code in-browser
  • My code contains descriptive comments
  • I've added tests where applicable, and...
  • I can run the unit tests successfully.

@ipranjal ipranjal marked this pull request as draft September 22, 2024 00:41
@ipranjal
Copy link
Contributor Author

WIP on #3029
cc : @mediaformat

@ipranjal ipranjal marked this pull request as ready for review September 22, 2024 04:52
@ipranjal
Copy link
Contributor Author

This concludes minimum integration of httpFoundation , ill continue working on changing all our superglobals with ->request()
All feedback and review are welcome @mediaformat @benwerd

@ipranjal ipranjal marked this pull request as draft September 23, 2024 02:56
@lindner
Copy link
Collaborator

lindner commented Oct 15, 2024

What's holding this up?

My only comment is that we could go all-in on Symfony and just use that instead of wrapping Request/Response?

https://symfony.com/doc/current/setup.html

That could replace a lot of the CLI and dev server/testing infra that is currently unmaintained...

@ipranjal
Copy link
Contributor Author

ipranjal commented Oct 16, 2024

@lindner currently there needs to be little bit of work done on the current session class of know to replace $_SESSSION super globals that's the only thing that is holding back this pull request.
Currently I have successfully

  1. Removed all $_SERVER and replaced it with request()->server
  2. Removed all $_REQUEST and replaced it with request()->request
  3. Removed all header() call and replaced it with request()->header / RedirectResponse()
  4. Removed all $_POST and replaced it with request()->request
  5. Removed all $_FILES and replaced it with request()->files
  6. Removed all echo call outside template and replaced it with response()->setContent()
  7. Removed Toro and made routing compatible with HttpFoundation
  8. Make onboarding compatible with HttpFoundation
  9. Modified Bonita core to use response()->setContent()
  10. Test everything on my local known instance

The last bit remaining is Removing all $_SESSION variables that needs a little bit of work as I need to integrate symfony session with current IDNO/Session .

Also while doing this I am finding few small bugs/improper way of handling things which I need to patch as part of this transition.

@lindner I agree ill remove the wrapper and directly use Symfony Request/Response , thanks for suggestion.

While I am finishing up Session integration, I would love to know thoughts of @benwerd on this PR

PS: I am a full time college student and work part time as freelance programmer , had a busy week did not get time to pach $_SESSSION yet, will finish this by tommrow most probably

@ipranjal ipranjal marked this pull request as ready for review October 16, 2024 11:29
@ipranjal
Copy link
Contributor Author

@lindner @benwerd PR ready to be reviewed and merged

Copy link
Collaborator

@lindner lindner left a comment

Choose a reason for hiding this comment

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

First pass, will delve deeper later today.

@@ -1,4 +1,4 @@
<link href="<?php echo \Idno\Core\Idno::site()->config()->getStaticURL() ?>css/<?php echo $this->getModifiedTS('css/known.min.css'); ?>/known.min.css" rel="stylesheet">
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is removing cache-busting for this file.

}
}
// if (!defined('KNOWN_UNIT_TEST')) {
// if (!empty($_SERVER['PHP_SELF'])) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove this instead of commenting please

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