Committed and pushed 34fa591813d to 11.x and febc710f53b to 11.2.x. Thanks!
Backported to 11.2.x because this is a bugfix.
alexpott → made their first commit to this issue’s fork.
I've updated the contribution record for this issue.
I've updated the issue summary and CR with the new name.
Added an MR comment. Sorry I should have spotted this earlier but the method name is problematic as it does nothing to address security issues with unserialize even though it sounds like it does.
@mondrake done that and updating to use ->commitOrRelease() proves we have a problem. The callbacks are only triggered on transaction destruction - not when ->commitOrRelease() processes the root commit.
This made it into 11.2.x-rc1 so is in the 11.2.x releases. Updating the CR.
This is fixed on 11.3.0 by ✨ Allow explicit COMMIT in Transaction objects Active = this issue could add test coverage.
Yep I've just run into this bug... it's because we fire the callbacks before we've emptied the stack and it is definitely a bug.
alexpott → created an issue.
alexpott → created an issue.
This is ready for review now.
Fixed up the MR after recent changes...
Fixed up the MR after the latest merges
I'll update this once https://www.drupal.org/project/decoupled_router/issues/3543594 📌 Test redirect.settings:passthrough_querystring for internal and external redirects Active lands as that one conflicts with this one.
📌 Test redirect.settings:passthrough_querystring for internal and external redirects Active rewrites the redirect event listener so this will be addressed once that one has landed.
alexpott → created an issue.
Committed and pushed 29704ed8d21 to 11.x and 74feffe03e3 to 11.2.x and d60071fb257 to 10.6.x and cbd00a5f38a to 10.5.x. Thanks!
Committed a22092a and pushed to 11.x. Thanks!
@claudiu.cristea I've creditted everyone on that issue.
alexpott → created an issue.
alexpott → created an issue.
Yeah and it is going to be a bit painful... because atm services get instantiated during the installer before config is imported and rely on the current behaviour.
Okay so resolved and redirect urls will obey the absolute setting but the urls inside jsonapi and entity will remain absolute. The one that is interesting is the canonical URL. I would guess that the front end think it exposes the canonical URL for the entity and not the backend.
Obviously something that is interesting about the memory cache backend adding for assets and admin links is that I suspect their entries are not covered by the cache tag invalidator...
So perhaps all that needs changing here is that cache.memory should become a more specific name... inline with
cache.access_policy_memory:
class: Drupal\Core\Cache\CacheBackendInterface
tags:
- { name: cache.bin.memory, default_backend: cache.backend.memory.memory }
factory: ['@cache_factory', 'get']
arguments: [access_policy_memory]
I think we're really close but when reviewing the code I realised we should add the cache tag info to the ThemeSettings object and use that. It'll nicely encapsulate the tags required for the memory cache.
Also we're missing a BC layer for drupal_static_reset('theme_get_setting');. The drupal_static() usage is a little more tricky. Not sure what to do about that.
Also adding cache.memory here is interesting. For other memory caches we've done stuff like
# Set up a cache chain for asset caching as the same assets may be
# requested several times, non-public as they are not meant to be reused.
cache.asset_memory:
class: Drupal\Core\Cache\MemoryCache\MemoryCache
arguments: ['@datetime.time']
public: false
and
system.module_admin_links_memory_cache:
class: Drupal\Core\Cache\MemoryCache\MemoryCache
arguments: ['@datetime.time']
Should we really be adding a generic one? I'm not sure about that.
I agree 📌 Support redirects to non-content entity pages for example a view Active has fixed this. That issue was a duplicate of this one.
@berdir suggested using the core.extension cache tag and then we wouldn't need the listener at all. Makes sense to me.
Added some review comments to the MR - we need to ensure uninstalled themes are removed from the memory cache too.
Implemented this...
What is interesting is that \Drupal\Core\Config\ConfigInstaller::createConfiguration() does not use the config factory so it is unaffected. In my mind this calls into question whether we actually need the method to create configuration. Let's see what the deprecations bring up... maybe creating config entities will need some changes...
I think it should, for example content editors can delete path aliases in a workspace and create redirects for them, and it would be good to notice any mistakes in this process.
But what happens if those 404s are only 404s in the workspace... they'll show up on the report even when a user is not in the workspace.
I'm wondering about how Redirect's submodules play with this. For example, should the 404 recorder be recording when in a workspace... I guess that's not really a revisionable issue... but we should at least have a think and check to see if there is an issue open already.
I think we just missing test coverage of the workspace version of finding by hash and then we're good. Maybe we need to look for more queries like the one in \Drupal\redirect\RedirectRepository::findMatchingRedirect() - i.e. a direct SQL query on the redirect table.
@nicxvan I'm not sure that this path as been deprecated... we're still doing things like $extension_discovery = new ExtensionDiscovery($this->root, FALSE, []); and passing FALSE to $use_info_parser will cause it to use this code path.
That said I stand by my comment in #6 ... we should not make this functionality generic so going to close won't fix because there's no been any call for this functionality in the years since this was opened.
Opened 📌 Only register decoupled_router.redirect_path_translator.subscriber if the Redirect module is installed Active to only have the redirect event when redirect is around. If that landed first then this one could do better re-injecting the repository.
#9 exactly. It's an additional argument - this module was kinda working around 🐛 Invalidate cached responses when new redirects are added Active - maybe it was not actually tested :) it is now.
alexpott → created an issue.
@mglaman they are not broken - given this actually works on the old redirect repository as we're only passing in an extra parameter... so I could change it to a warning. How does that sound. It would make cacheability stuff less good than it is now. The alternative is to swap out the implementations depending on what redirect version you have. Makes for complicated testing though.
This is now ready for review.
This got fixed by [#2a5036c]
I think if we limit this to redirects this is quite easy to support and inline with the support for external redirects too... see 📌 Support redirects to non-content entity pages for example a view Active - I think this issue is a duplicate of that one.
alexpott → created an issue.
alexpott → created an issue.
Added some review questions. I very concerned about the hash value and \Drupal\redirect\Entity\Redirect::preSave()
alexpott → created an issue.
This looks great - nice test @mglaman. I merged in 2.x and fixed up PHPCS.
alexpott → made their first commit to this issue’s fork.
This will be fixed and tested by 🐛 Use RedirectRepository for matching redirects Active - note we need a release of redirect so that we can depend on a version that has 🐛 Invalidate cached responses when new redirects are added Active fixed.
alexpott → changed the visibility of the branch 3541101-only-drafts-plugin to hidden.
Okay looking at ✨ Add content moderation support, for preserving "published" state revisions Fixed it seems there is a use-case but the implementation leaves a bit to be desired. Let's fix it.
It also has a dependency on the content_moderation module as it does $revision_state = $revision->get('moderation_state')->getString(); but it does not check whether such a field exists or if content moderation is enabled.
What I think the plugin actually does is delete draft revisions older than the active revision - i.e. old revisions that have never been published.
I'm not sure this module should be supplying a plugin for that case -especially one that says After this time, draft revisions newer than the active revision can be deleted. The minimum age of revisions is always respected, regardless of other settings. Only draft revisions created after the active revision will be deleted. when it actually does something completely different.
Need a bit more information about how the queue is being processed when you get this error.
-
+++ b/src/Commands/AdvancedQueueCommands.php @@ -95,6 +95,57 @@ class AdvancedQueueCommands extends DrushCommands { + * @option timeout The maximum execution time for each queue. Defaults to 90 seconds.Do we need to be smarter here. If we have a max execution time setting in PHP it feels like we need to stop the script before it overshoots it. I think this is more problematic than the single queue processor...
-
+++ b/src/Commands/AdvancedQueueCommands.php @@ -95,6 +95,57 @@ class AdvancedQueueCommands extends DrushCommands { + if (extension_loaded('pcntl')) { + pcntl_async_signals(TRUE); + + pcntl_signal(SIGTERM, function () { + $this->processor->stop(); + }); + + pcntl_signal(SIGINT, function () { + $this->processor->stop(); + }); + }This doesn't need to be done in the loop.
Can we convert this to an MR? Thanks!
Plenty of merge conflicts now.
Let's get this fixed.
Actually we have this functionality in 8.x-1.x and we're trying to use transactions to do it properly but we've not done it right. Going to use this issue to fix it so I can credit @nvahalik for their original work here.
Afaics this is still an issue in 8.x-1.x so moving there.
In 8.x-1.x the drush command needs a queue ID so this is outdated as 7.x-1.x is no longer supported.
In 8.x-1.x you enqueue a job object and this is updated with the job ID. As 7.x-1.x is unsupported this can be closed as outdated.
Afaics we do this correctly now in 8.x-1.x - see
if (extension_loaded('pcntl')) {
pcntl_async_signals(TRUE);
pcntl_signal(SIGTERM, function () {
$this->processor->stop();
});
pcntl_signal(SIGINT, function () {
$this->processor->stop();
});
}
Closing as outdated as 7.x-1.x is no longer supported.
This sounds like a good idea and I don't think there is a corollary in the 8.x-1.x issue queue so going to move it there. Obvsiously the existing work will need to be completely redone.
This view no longer exists - we're using a config list builder for the equivalent page in 8.x-1.x
7.x-1.x is unsupported and a similar issue exists for 8.x-1.x with work on it so going to close in favour of that one. See ✨ Create a way to empty a queue Active
This view no longer exists in 8.x-1.x - the list of queues is done using a config entity list builder and 7.x is no longer supported.
This text no longer exists in the 8.x-1.x version and the 7.x version is no longer supported.
This code is no longer present in the 8.x-1.x version therefore and that version is no longer supported so closing this issue.
As this feature request has not received any support or work for nearly 8 years I'm closing this. one.
alexpott → created an issue.