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

Context name not populated when manually triggering the context #592

Open
TheoD02 opened this issue Dec 17, 2024 · 2 comments
Open

Context name not populated when manually triggering the context #592

TheoD02 opened this issue Dec 17, 2024 · 2 comments

Comments

@TheoD02
Copy link
Contributor

TheoD02 commented Dec 17, 2024

For certain checks, I need to determine which context is authorized by its name. To achieve this, I declared a context using the AsContext attribute:

#[AsContext(name: 'symfony')]
function symfony_context(): Context
{
    return root_context()
        ->withName('symfony')
        ->withWorkingDirectory(ROOT_DIR . '/app')
        ->withData([
            'registry' => 'xxx',
            'image' => 'xxx',
        ]);
}

Here, the name 'symfony' is set and accessible using code like:

$contextName = context()->name;

However, when I attempt to manually resolve the name like this:

$deployContextNames = [symfony_context()->name, frontend_context()->name];
// or
$deployContextNames = [context('symfony')->name, context('frontend')->name]

The resulting array is:

['', '']

The names are empty because, by default, the name property in the Context constructor is not populated. While calling withName('...') works, the constructor comment states:

// Do not use this argument, it is only used internally by the application

This raises concerns about potential side effects or breaking behavior if withName() is explicitly used by users.


Current Behavior:

  • Context names remain empty when manually instantiating or resolving contexts.
  • The attribute #[AsContext(name: '...')] does not auto-populate the name of context instance.

Expected Behavior:

  • The context name should be auto-populated from the AsContext attribute upon instantiation.

Possible Solutions:

  1. Automatically populate the name property when instantiating a context if the AsContext attribute is set with a name.
  2. Allow explicit use of withName() without concerns, and clarify whether its use is officially supported.

Environment:

  • Castor version: 0.21.0
  • PHP version: 8.4.0

Additional Notes:
It’s unclear if manually setting the name or withName() could cause issues. Is there any risk in doing so, or can it be safely permitted?

@pyrech
Copy link
Member

pyrech commented Dec 30, 2024

Thanks for the report.

I agree that it may be misleading that the context name is not always available.

However, when I attempt to manually resolve the name like this:
$deployContextNames = [symfony_context()->name, frontend_context()->name];

In your reproducer, calling symfony_context()->name from a task correctly returns the symfony name so not sure if I miss something

$deployContextNames = [context('symfony')->name, context('frontend')->name]

Here, I reproduce the issue and it can be fixed with a simple modification in the ContextRegistry, by calling withName before registering the context. I will open an PR soon about it.

@TheoD02
Copy link
Contributor Author

TheoD02 commented Dec 30, 2024

In your reproducer, calling symfony_context()->name from a task correctly returns the symfony name so not sure if I miss something

Yes sorry, probably I missed something when writing and used the example case I have in my project and not the case I exposed in issue.

But for example this context:

#[AsContext(default: true, name: 'my_default')]
function defaultContext(): Context
{
    return new Context()->withName('my_default_runtime');
}

It will output :

// here it should be one or another ? which have the priority ?
array:3 [
  "context()->name" => "my_default"
  "context('my_default')->name" => "my_default_runtime"
  "defaultContext()->name" => "my_default_runtime"
]

But the same context (no call to withName(...)or name in constructor:

#[AsContext(default: true, name: 'my_default')]
function defaultContext(): Context
{
    return new Context();
}

Will output:

// Here it should be my_default everywhere
array:3 [
  "context()->name" => "my_default"
  "context('my_default')->name" => ""
  "defaultContext()->name" => ""
]

I think if possible, we need to only have one way to define a name for context or we need to clarify them

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

No branches or pull requests

2 participants