-
Notifications
You must be signed in to change notification settings - Fork 31
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
Support for eZ kernel 7.0 #122
Conversation
Any idea what else will be needed? Asking as it might not need a new major version unless it involves breaks. |
At the very least, support for the new HTTP cache bundle. I think that it's a breaking change, but not sure. |
@@ -31,6 +31,7 @@ public function process(ContainerBuilder $container) | |||
new Reference('ezplatform.http_cache.purge_client'), | |||
new Reference('ezpublish.api.service.inner_content'), | |||
new Reference('ezpublish.http_cache.event_dispatcher'), | |||
new Reference('ezpublish.api.repository'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is also relevant for upcoming 6.7.7, 6.12.1 and 6.13.0.
Can code be done in different way to avoid having to set all arguments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to find a way. This is just a playground for now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a way to fix this any other way :/
The problem is that by this point any reference to old ezpublish.http_cache.purger.instant
is removed from the container and we simply do not know the list of arguments.
@@ -69,7 +69,7 @@ public function all() | |||
return; | |||
} | |||
|
|||
$this->cache->clear(); | |||
//$this->cache->clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrerom I'm going to need some help with this one, or rather, with any commented out call to cache client here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, should clear by tag, will simplify this a lot and maybe also avoid some of the broad clearings.
well first we need to decide if we are going to have a branch only supporting 7.0 or have two adapters here being injected depending on version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all for keeping it simple. 2.0 legacy bridge supporting only kernel 7.0.
If needed, this adapter could be backported than to 1.x.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll push commit for this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget other commented out calls, those are all I didn't know what to do with :)
@emodric pushed, not tested yet here, but on theoretical side only relevant todo left there is in regards to clearing type cache on type group changes. The user one is most likely not needed given how it is used. |
@andrerom Thanks a lot! It looks okay with only a reading of the code. We can always iterate later if something is missing :) |
9e2801b
to
ec3ded4
Compare
@andrerom In this state it is okay with me to merge. WDYT? I was thinking of removing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, tested?
So far so good, no issues. |
Just an unlock in
composer.json
for now, but obviously will need some more work :)