Skip to content

Generate Access Interceptor Proxy - #43

Open
kletord wants to merge 15 commits into
masterfrom
warmp
Open

Generate Access Interceptor Proxy#43
kletord wants to merge 15 commits into
masterfrom
warmp

Conversation

@kletord

Copy link
Copy Markdown
Contributor

Time to open this PR.

Main change: remove the value holder proxy to create an extended class with the interceptor.

This fix some bug and limitation and add another bunch of limitation.

Removed limitations:

  1. Inner object method calls respected annotations behavior
  2. protected method can be annotated

New limitations:

  1. Unable to proxify a factory service definition inside Symfony

Also this PR add a few errors when an impossible operation is asked and add a cache warmer to generate proxies.

I kept the term proxy but is not a proxy anymore ;-)

@kletord
kletord requested a review from siduxMarch 25, 2024 09:30
@github-actions

Copy link
Copy Markdown

Issued by Coverage Checker:

public function generate(\ReflectionClass $originalClass, ClassGenerator $classGenerator, array $proxyOptions = [])
{
if (!\array_key_exists('methods', $proxyOptions)) {
throw new \InvalidArgumentException(sprintf('Generator %s needs a methods proxyOptions', __CLASS__));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
thrownew \InvalidArgumentException(sprintf('Generator %s needs a methods proxyOptions', __CLASS__));
thrownew \InvalidArgumentException(sprintf('Generator %s needs a property "methods" in proxyOptions', __CLASS__));

also I'm not sure what you meant with this errors, that the array proxyOptions must have a key 'methods' in it or if the key 'methods' doesn't exist in the array, the Generator class that extends this one must have a method "getProxyOptions()" or something similare ?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

proxyOptions must have a "methods" key

$factoryDefinition->setTags($definition->getTags());

if ($definition->getFactory() !== null) {
$this->compiler->log($this, "Service {$taggedServiceName} is not compatible with service proxy");

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Make an error

{
foreach ($this->proxies as $proxy) {
if ($proxy instanceof LazyLoadingInterface && !$proxy->isProxyInitialized()) {
$proxy->initializeProxy();

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

add continue

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

What is LazyLoadingInterface doesn't exist

Comment threadsrc/FrameworkBridge/Symfony/CacheWarmer/ProxyCacheWarmer.php
Comment threadtests/Double/Stub/Cache/ClassWithInvalidateCacheAttributes.php Outdated
Comment threadtests/Double/Stub/FinalClass.php
Comment threadsrc/ProxyFactory.php
{
$instanceRef = new \ReflectionObject($object);
$methods = $instanceRef->getMethods(\ReflectionMethod::IS_PUBLIC);
$instanceRef = new \ReflectionClass($class);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

although scanning all methods of a class permits us to throw errors on decorated private methods, the cost of loading annotation for each method maybe high in dev env

@kletord
kletordforce-pushed the warmp branch 2 times, most recently from a1e73ed to 48b6ea4CompareApril 15, 2024 12:59
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@kletord@sidux@arnaud-23