Skip to content

refactor: AuthToken/AuthJWT Config Loading by Initializing in Constructor - #1252

Open
datamweb wants to merge 3 commits into
codeigniter4:developfrom
datamweb:refctor-hmac-config
Open

refactor: AuthToken/AuthJWT Config Loading by Initializing in Constructor#1252
datamweb wants to merge 3 commits into
codeigniter4:developfrom
datamweb:refctor-hmac-config

Conversation

@datamweb

@datamwebdatamweb commented Feb 16, 2025

Copy link
Copy Markdown
Collaborator

Description
AuthToken and AuthJWT Config Loading by Initializing in Constructor

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@datamwebdatamweb added the refactor Pull requests that refactor code label Feb 16, 2025
@datamwebdatamweb changed the title refactor: Optimize AuthToken Config Loading by Initializing in Constructorrefactor: Optimize AuthToken/AuthJWT Config Loading by Initializing in ConstructorFeb 16, 2025

@paulbalandanpaulbalandan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you have a benchmark for this change?

@datamweb

Copy link
Copy Markdown
CollaboratorAuthor

Do you have a benchmark for this change?

The presence of Optimize probably led to this question. It seems I made a mistake in choosing it, as my main goal in this configuration was to prevent the repeated use of config('AuthToken') and config('AuthJWT'), nothing more.

But now I’m curious—does this change actually impact performance or not? Your benchmark results could be really helpful! @paulbalandan If possible, please share them so we can analyze them together.

@paulbalandan

Copy link
Copy Markdown
Member

Yes, the word Optimize in your PR title made me curious about the impact to performance since we will now be loading the configs everytime a class is instantiated. I just remembered one time when I did a similar change to the Seeder class instantiating Faker's Generator everytime in the constructor caused a massive slowdown.

@michalsn

Copy link
Copy Markdown
Member

Are we actually calling an attempt method multiple times per request?

@datamwebdatamweb changed the title refactor: Optimize AuthToken/AuthJWT Config Loading by Initializing in Constructorrefactor: AuthToken/AuthJWT Config Loading by Initializing in ConstructorFeb 27, 2025
@datamweb

Copy link
Copy Markdown
CollaboratorAuthor

Are we actually calling an attempt method multiple times per request?

I didn't quite understand your question, and I'm not sure how it relates to this PR. Could you please clarify?

@michalsn

Copy link
Copy Markdown
Member

Sorry, your reason for this change was described as:

my main goal in this configuration was to prevent the repeated use of config('AuthToken') and config('AuthJWT')

So my question is. is this really the case? Are we actually calling these configs multiple times, or only once per request?

@datamweb

Copy link
Copy Markdown
CollaboratorAuthor

is this really the case? Are we actually calling these configs multiple times, or only once per request?

In fact, the configuration is only called once per request. Therefore, it seems this change wouldn’t have a significant impact on performance. 100%

Which one do you find more suitable and why?"

First method:

<?phpdeclare(strict_types=1);
class MyClass
{
publicfunction__construct() {
$this->config = config('Xconfig');
}
publicfunctionY()
{
$this->config;
}
publicfunctionX()
{
$this->config;
}
}

Second method:

<?phpdeclare(strict_types=1);
class MyClass
{
publicfunction__construct() {}
publicfunctionY()
{
$config = config('Xconfig); } public function X() { $config = config('Xconfig);
}
}

@michalsn

Copy link
Copy Markdown
Member

Ok, sorry - that was my mistake. I thought we call this config only in one method. But now I see we call it several times.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactorPull requests that refactor code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@datamweb@paulbalandan@michalsn