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

BUGFIX: Make cache application identifier configurable #1457

Merged
merged 5 commits into from
Nov 23, 2018

Conversation

kitsunet
Copy link
Member

@kitsunet kitsunet commented Nov 21, 2018

In multiple permutations, we tried to fix problems with cache identifier
uniqueness in cache backends that are shared like apcu or memcache.
In earlier days it included the PHP_SAPI and then in more recent times
the context and root path. With the refactoring of caches, these two
became the hardcoded applicationIdentifier which can be used by
any backend to add more specificity to cache identifiers.

It turns out that the root path doesn't work well for some environments
and can result in bugs when used with eg. the PdoBackend and a
deployment that changes the root path (typical Surf or Deployer).

The only backward compatible way to fix this was to make the
applicationIdentifier configurable with a default that matches the
previously hardcoded values. That way nothing changes in existing
installations but if the bug appears it can be easily fixed.

In multiple permutations we tried to fix problems with cache identifier
uniqueness in cache backends that are shared like apcu or memcache.
In earlier days it included the PHP_SAPI and then in more recent times
the context and root path. With the refactoring of caches these two
became the hardcoded `applicationIdentifier` which can be used by
any backend to add more specifity to cache identifiers.
It turns out that he root path doesn't work well for some enviroments
and can result in bugs when used with eg. the PdoBackend and a
deployment that chnages the root path (typical Surf or Deployer).
The only backwards compatible way to fix this was to make the
applicationIdentifier configurable with a default that matches the
previously hardcoded values. That way nothing changes in existing
installations but if the bug appears it can be easily fixed.
@jonnitto
Copy link
Member

@kitsunet The tests are still failing…

@kitsunet
Copy link
Member Author

Yep, yep, I see. during rebasing I coudln't run the tests so I hoped it would work, but I kind of expected that functional tests might also have a problem...

@kdambekalns
Copy link
Member

Amended documentation. The failure comes from the compile step and is caused by the arguments in the object configuration being empty. The reason seems to be that the factory configuration is used, according to the configurationSourceHint seen below.:

/…/Packages/Framework/Neos.Flow/Classes/ObjectManagement/Configuration/ConfigurationBuilder.php:402:
class Neos\Flow\ObjectManagement\Configuration\Configuration#1482 (12) {
  protected $objectName =>
  string(32) "Neos\Cache\CacheFactoryInterface"
  protected $className =>
  string(28) "Neos\Flow\Cache\CacheFactory"
  protected $packageKey =>
  string(9) "Neos.Flow"
  protected $factoryObjectName =>
  string(0) ""
  protected $factoryMethodName =>
  string(6) "create"
  protected $scope =>
  int(2)
  protected $arguments =>
  array(0) {
  }
  protected $properties =>
  array(0) {
  }
  protected $autowiring =>
  int(1)
  protected $lifecycleInitializationMethodName =>
  string(16) "initializeObject"
  protected $lifecycleShutdownMethodName =>
  string(14) "shutdownObject"
  protected $configurationSourceHint =>
  string(92) "configuration of package Neos.Flow, definition for object "Neos\Cache\CacheFactoryInterface""
}
Could not autowire required constructor argument $applicationIdentifier for singleton class Neos\Flow\Cache\CacheFactory.
Check the type hint of that argument and your Objects.yaml configuration.

  Type: Neos\Flow\ObjectManagement\Exception\UnresolvedDependenciesException
  Code: 1298629392
  File: Packages/Framework/Neos.Flow/Classes/ObjectManagement/Configuration/Configu
        rationBuilder.php
  Line: 403

Open Data/Logs/Exceptions/20181122082231a876f8.txt for a full stack trace.

properties:
logger:
object:
factoryObjectName: Neos\Flow\Log\PsrLoggerFactoryInterface
Copy link
Member

Choose a reason for hiding this comment

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

No such thing as Neos\Flow\Log\PsrLoggerFactoryInterface in Flow 4.3

@kdambekalns
Copy link
Member

Ok, it get's shady here… when changing Objects.yaml and instead of 3 using 2 as argument index, the var_dump() suddenly emits the correct setup… as in:

/…/Packages/Framework/Neos.Flow/Classes/ObjectManagement/Configuration/ConfigurationBuilder.php:402:
class Neos\Flow\ObjectManagement\Configuration\Configuration#1408 (12) {
  protected $objectName =>
  string(28) "Neos\Flow\Cache\CacheFactory"
  protected $className =>
  string(28) "Neos\Flow\Cache\CacheFactory"
  protected $packageKey =>
  string(9) "Neos.Flow"
  protected $factoryObjectName =>
  string(0) ""
  protected $factoryMethodName =>
  string(6) "create"
  protected $scope =>
  int(2)
  protected $arguments =>
  array(2) {
    [1] =>
    class Neos\Flow\ObjectManagement\Configuration\ConfigurationArgument#1465 (4) {
      protected $index =>
      int(1)
      protected $value =>
      string(17) "Neos.Flow.context"
      protected $type =>
      int(2)
      protected $autowiring =>
      int(1)
    }
    [2] =>
    class Neos\Flow\ObjectManagement\Configuration\ConfigurationArgument#1466 (4) {
      protected $index =>
      int(2)
      protected $value =>
      string(37) "Neos.Flow.cache.applicationIdentifier"
      protected $type =>
      int(2)
      protected $autowiring =>
      int(1)
    }
  }
  protected $properties =>
  array(0) {
  }
  protected $autowiring =>
  int(1)
  protected $lifecycleInitializationMethodName =>
  string(16) "initializeObject"
  protected $lifecycleShutdownMethodName =>
  string(14) "shutdownObject"
  protected $configurationSourceHint =>
  string(88) "configuration of package Neos.Flow, definition for object "Neos\Flow\Cache\CacheFactory""
}

For some reason the arguments need to be defined for "both objects".

The logger factory does not need to be configured.
@kdambekalns
Copy link
Member

That fixes it, @kitsunet – but I am not entirely sure why. Could it be that there are places where the cache factory is fetched by different names?

@kdambekalns kdambekalns dismissed their stale review November 22, 2018 08:47

Changed it myself, needs re-review

@kitsunet kitsunet changed the base branch from 4.3 to 5.0 November 22, 2018 09:53
@kitsunet kitsunet changed the base branch from 5.0 to 4.3 November 22, 2018 09:53
Copy link
Member Author

@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.

Alright, that works for me. I guess I should do the upmerges and fix the object configuration along the way. In 5.0 AFAIK the autowiring check was changed to not cause this kind of problem so the whole config can be removed again, as in my original change on master.

Copy link
Member

@daniellienert daniellienert left a comment

Choose a reason for hiding this comment

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

Great! Thanks for adding the hint to the documentation.

@jonnitto jonnitto merged commit 909fe01 into neos:4.3 Nov 23, 2018
@albe albe deleted the task/fix-application-identifier-43 branch November 23, 2018 13:39
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.

4 participants