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

FEATURE: Introduce EEL tracer for handling Neos9 deprecations #3386

Merged

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Aug 9, 2024

Related neos/neos-development-collection#5022

In todays weekly @bwaidelich and me discussed a concrete way how to log deprecations like node.indentifier in Neos 8.4 and 9.0

The idea is to add a tracer to eel, that will be implemented in Fusion. Technically we would need to add an abstraction to Neos.Fusion as well to not access the Node from there as this is architecturally illegal but to simplify the code and in light that this is just considered for temporary time we propose to implement it as such:

<?php

namespace Neos\Fusion\Core;

use Neos\Flow\Annotations as Flow;
use Psr\Log\LoggerInterface;

final class Neos9RuntimeMigrationTracer implements EelInvocationTracerInterface
{
    /** @Flow\Inject */
    protected LoggerInterface $logger;

    private const DEPRECATED_NODE_PROPERTIES = [
        'identifier' => true,
        'nodetype' => true,
        // ...
    ];

    public function __construct(
        private readonly string $eelExpression,
        private readonly bool $showMercy
    ) {
    }

    public function recordPropertyAccess(object $object, string $propertyName): void
    {
        if (
            $object instanceof \Neos\ContentRepository\Domain\Model\Node
            && array_key_exists(strtolower($propertyName), self::DEPRECATED_NODE_PROPERTIES)
        ) {
            $this->logDeprecationOrThrowException(
                sprintf('"node.%s" is deprecated in "%s"', $propertyName, $this->eelExpression)
            );
        }
    }

    public function recordMethodCall(object $object, string $methodName): void
    {
    }

    private function logDeprecationOrThrowException(string $message): void
    {
        if ($this->showMercy) {
            $this->logger->debug($message);
        } else {
            throw new \RuntimeException($message);
        }
    }
}

and instantiate this Neos9RuntimeMigrationTracer (name tbd) in \Neos\Fusion\Core\Runtime::evaluateEelExpression

- return EelUtility::evaluateEelExpression($expression, $this->eelEvaluator, $contextVariables);
+ $tracer =.$this->settings['enableDeprecationTracer'] ? new Neos9RuntimeMigrationTracer($expression, $this->settings['strictEelMode'] ?? false) : null;
+ return EelUtility::evaluateEelExpression($expression, $this->eelEvaluator, $contextVariables, $tracer);

Upgrade instructions

Review instructions

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

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

Before/after performance in a reasonably big site please ;) I think as small as this is, it will be a performance hit.

@mhsdesign
Copy link
Member Author

Of course if Fusion implements a stupid tracer that is way too slow by doing to many things there will be a performance hit, BUT as i have drawn out above we want to set the tracer fully to null in production and thus the optional chaining will just be a true noop:

$this->tracer?->recordMethodCall($this->value, $method);

I also considered introducing some NoopTracer but that would be slower i guess.

@mhsdesign mhsdesign changed the base branch from 9.0 to 8.4 August 19, 2024 11:25
@github-actions github-actions bot added 8.4 and removed 9.0 labels Aug 19, 2024
Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

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

Performance discussed, I am fine with that ✅

I woulnd't call it showMercy, but that is not part of this change so fine as the example :D

I guess lets bring it in...

@kitsunet kitsunet merged commit ee5d017 into neos:8.4 Sep 12, 2024
6 of 7 checks passed
@mhsdesign mhsdesign deleted the feature/eel-tracer-for-neos-9-deprecations branch September 24, 2024 17:44
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Sep 24, 2024
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Oct 30, 2024
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Oct 30, 2024
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Oct 30, 2024
neos-bot pushed a commit to neos/fusion that referenced this pull request Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants