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.

Sign up for GitHub

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 wakeup call in CompileInjector #111

Merged
merged 3 commits into from
May 10, 2024
Merged

Add wakeup call in CompileInjector #111

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
50e063e
Add wakeup call in CompileInjector
koriym May 8, 2024
bac220d
Ignore __wakeup method in code coverage
koriym May 9, 2024
23610e9
Add suppression for Psalm warning
koriym May 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/CompileInjector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,13 @@ public function getInstance($interface, $name = Name::ANY)
return $this->singletons[$dependencyIndex];
}

/** @psalm-suppress DocblockTypeContradiction */
if ($this->functions === null) {
// @codeCoverageIgnoreStart
$this->__wakeup();
// @codeCoverageIgnoreEnd
}
Comment on lines +137 to +141
Copy link
Contributor

@coderabbitai coderabbitai bot May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Address the type contradiction and logic of the null check on this->functions.

The addition of a null check on this->functions suggests that it can be null. However, the type declaration of this->functions as an array of callables implies it should never be null. This contradiction might indicate an issue with either the type declaration or the logic of the null check. Consider revising the type declaration if null is a valid state or adjusting the logic to handle this scenario differently.

Copy link
Member Author

Choose a reason for hiding this comment

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

@coderabbitai I agree, but there are cases where __wakeup is not called when serialising or unserialising this class, and this is a countermeasure to that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@koriym, thank you for clarifying the situation with the __wakeup method not being called automatically in certain serialization or deserialization scenarios. This indeed sounds like a practical safeguard.

Given this context, it might be beneficial to ensure that the serialization and deserialization processes are aligned with PHP's expected behaviors, where __wakeup should ideally be invoked automatically. If this issue is recurring or if there are specific conditions under which __wakeup fails to be called, it could be valuable to document these cases or consider a review of the overall design to address potential inconsistencies.

If you think it would be helpful, we could look into documenting these scenarios more formally or even revisiting the design to ensure robustness without needing manual interventions. Let me know how you would like to proceed!

Copy link
Member Author

Choose a reason for hiding this comment

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

@coderabbitai See koriym/BEAR.SpikeProject#1

Copy link
Contributor

Choose a reason for hiding this comment

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

@koriym, thank you for sharing the link to the GitHub issue. It provides valuable insights into the problem you're facing with serialization and deserialization in the context of the CompileInjector.

Issue Summary:

The issue describes a scenario where the injector cache fails to verify, leading to a PHP warning. This seems to occur when using the bear.compile command with the MyVendor\MyProject in a production environment. The warning points to a failure in verifying the injector cache, similar to another issue reported in the BEAR.Package repository.

Key Points from the Issue:

  1. Serialization Issue: The __wakeup method is not being called as expected during the unserialization process. This is crucial because, as noted in the issue, PHP's unserialize() function treats the result as a creation of a 'new' object, and thus __wakeup is only called if the object is directly created as a result of unserialization.
  2. Current Workarounds Proposed:
    • A) Calling __wakeup externally from ResourceStorage (a temporary fix).
    • B) Phasing out the use of magic methods in CompileInjector to ensure that the absence of these method calls does not affect functionality.

Suggested Actions:

Given the complexity and the recurring nature of this issue, it might be beneficial to consider a more robust solution that either ensures __wakeup is reliably called or redesigns the system to not rely on it. Here are a couple of suggestions:

  • Documenting the Issue: Enhance the documentation around these serialization behaviors to ensure that developers are aware of these quirks and the necessary workarounds.
  • Code Review and Refactoring: It might be worth reviewing the use of serialization and magic methods across the project to identify potential improvements or alternatives that could make the system more resilient and less dependent on these mechanisms.

If you need further analysis or assistance in implementing these suggestions, feel free to let me know!

Analysis chain
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Fetch the details of the GitHub issue mentioned by the user.
gh issue view https://github.com/koriym/BEAR.SpikeProject/issues/1 --json title,body

Length of output: 1835

Copy link
Contributor

Choose a reason for hiding this comment

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

@koriym, thank you for sharing the link to the GitHub issue. It provides valuable insights into the problem you're facing with the serialization and deserialization process in the CompileInjector class.

Issue Summary:

The issue titled "PHP Warning: Failed to verify the injector cache" describes a scenario where the injector cache fails to verify, leading to a PHP warning. This occurs when attempting to compile with the command ./vendor/bin/bear.compile 'MyVendor\\MyProject' prod-app ./. The warning points to a failure in verifying the injector cache, which is linked to the __wakeup method not being called as expected during the unserialization process.

Key Points from the Issue:

  • The __wakeup method is not being called automatically when expected, particularly when objects are nested within serialized data. This is because PHP's unserialize() function does not automatically trigger __wakeup for already unserialized objects nested within the data.
  • The issue suggests two potential approaches to address this:
    • A) Call __wakeup externally from ResourceStorage (a workaround approach).
    • B) Eliminate the reliance on magic methods like __wakeup in CompileInjector (a more permanent solution).

Given this context, it seems prudent to consider these approaches to ensure robustness in the serialization and deserialization processes. If you would like, I can assist in drafting more detailed documentation on this behavior or help explore a redesign that minimizes reliance on PHP's magic methods, which seems aligned with your initial intent to avoid such dependencies.

Please let me know how you would like to proceed or if there's any other way I can assist!

Analysis chain
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Fetch the details of the GitHub issue mentioned by the user.
gh issue view https://github.com/koriym/BEAR.SpikeProject/issues/1 --json title,body

Length of output: 1835


[$prototype, $singleton, $injectionPoint, $injector] = $this->functions;
/** @psalm-suppress UnresolvableInclude */
$instance = require $this->getInstanceFile($dependencyIndex);
Expand Down
Loading