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

Separate Security Configuration #3703

Closed
wants to merge 49 commits into from

Conversation

mostafakhudair
Copy link
Contributor

@mostafakhudair mostafakhudair commented Oct 1, 2020

Creates new Config\Security class and make \CodeIgniter\Security\Security class depends on it

removes useless properties
renames some methods and properties
add new public methods getHeaderName() & getCookieName()
edits changes in user guide

  • Securely signed commits
  • Component(s) with PHPdocs
  • User guide updated
  • Conforms to style guide

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

While the move is good, we need to check for BC as removing properties from App config can be breaking for existing apps. Perhaps you can still persist it by supplying the values of the new Security config in the App constructor for the meantime, then hinting that these App config properties are deprecated in favor of Security config.

system/Common.php Outdated Show resolved Hide resolved
system/Config/Services.php Outdated Show resolved Hide resolved
system/Config/Services.php Outdated Show resolved Hide resolved
system/Config/Services.php Show resolved Hide resolved
system/Common.php Outdated Show resolved Hide resolved
system/Security/Security.php Outdated Show resolved Hide resolved
system/Common.php Show resolved Hide resolved
system/Filters/CSRF.php Outdated Show resolved Hide resolved
tests/system/Security/SecurityTest.php Show resolved Hide resolved
@mostafakhudair
Copy link
Contributor Author

@paulbalandan could you tell me why Rector fails

@paulbalandan
Copy link
Member

@paulbalandan could you tell me why Rector fails

Your docblocks should not use FQCN for classes. See the static analysis results for those docblocks.

@samsonasik
Copy link
Member

samsonasik commented Oct 21, 2020

@mostafakhudair while I appreciate your contribution, please don't change unrelated change (cosmetic changes) to the PR, like docblock and comment (That should be another specific pull request). It make harder to review, and harder to revert changes.

Also, please use meaningfull commit messages with specific task, better to squash them with group of task if they are steps,eg, the usecase for PR with title: "upgrade php requirement to 7.3 " can have the following commits steps:

  • bump php requirement to 7.3
  • remove reversed keyword/syntaxes in php 7.3

Until here, if there is no error, then stop. Improvement? Another pull request!

If above got error, eg: based on dependencies, eg: PHPUnit error, need upgrade to PHPUnit 9, then upgrade with new commit messages:

  • upgrade requirement: phpunit 9
  • update tests syntaxes to confirm with phpunit 9

No need to changes unrelated to the PR, really! So, that will be an easier to rollback in the future.

Thank you ;)

@mostafakhudair mostafakhudair marked this pull request as draft October 21, 2020 20:33
@mostafakhudair
Copy link
Contributor Author

New clear PR under processing

@mostafakhudair mostafakhudair deleted the patch-1 branch October 30, 2020 10:33
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.

3 participants