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.
@ankitv18 sorry I was already working on this and raised the minimum already - which imo is fine as 10.3 is not supported. I force pushed because your changes are now unnecessary and other changes are.
alexpott → created an issue.
alexpott → created an issue.
Given 7.x is no longer supported closing this issue. If someone wants to implement this on the currently supported module for Drupal 10 and 11 please open a new issue.
alexpott → created an issue.
Thanks @mauro_
alexpott → made their first commit to this issue’s fork.
This needs more changes. We're in a loop for redirect loop detection but that's part of the repository so it shouldn't be doing that here. Plus we should leverage 🐛 Invalidate cached responses when new redirects are added Active to get the cacheable metadata correct.
alexpott → made their first commit to this issue’s fork.
The code in question was mostly modified to work this way in #2845884: Redirect setRedirect only saves internal paths → - I think the use of isValid() to detect externalness is wrong. We should be using UrlHelper::isExternal(). I think there is a further follow-up to consider whether or not we should allow invalid external URLs to be set. In my opinion this should cause an error.
alexpott → created an issue.
alexpott → made their first commit to this issue’s fork.
I've removed tests/phpunit.xml because it's not really possible for a provide a working phpunit.xml for D10 and D11 so it's pointless.
alexpott → created an issue.
Re-titling issue to make it clear what is happening. I think dropping D9 support is okay because it is now unsupported.
These changes would mean we have to drop the Drupal 9 support. I think this seems fine but maybe we want a new major version to do that.
So one thing that's occurred to me and feels like a bit of a problem... this puts the onus of translatability of config actions on the recipe author and not the config action author and that feels kinda wrong. Because the same config action can be used by many different recipe authors.
I love to completely remove this and replace with indexes on json now we require json storage...
I've addressed @berdir's feedback - thanks for the review!
Totally agree that running multiple PHPStan jobs is a recipe for confusion and pain.
Will do the rest as asked as it all makes sense to me! Thanks for thinking about this @berdir.
@quietone thanks for the bisect. Went ahead with 🐛 Fix EntityQueryTest on SQLite and Postgres Active so we can close this one again.
Committing this so Postgres and SQLite are green again.
Committed and pushed 1b1133d1ff8 to 11.x and 71171df537a to 11.2.x and 23b79dfebd6 to 10.6.x and 1a0d6efea9d to 10.5.x. Thanks!
We already have sorts on other queries in these tests and we don't guarantee the result of an unsorted query so for me these changes are fine. Also both of these queries were added by 🐛 Entity queries querying the latest revision very slow with lots of revisions Needs work and would have failed on Postgres without the change to EntityQuery in that issue.
Opened 🐛 Fix EntityQueryTest on SQLite and Postgres Active to fix Postgres and SQLite
alexpott → created an issue.
berdir → credited alexpott → .
I've fixed the MR to redo the check if the change the source path and I've added test coverage to ensure that you can save a redirect that overrides a path still.
Added unit test and paired the patch back to the minimum necessary change.
I think this is a nice change that improves redirect validation and the ability to create redirects via an API.
// The node should retain the translations from the last default revision.
// @see \Drupal\Core\Entity\ContentEntityStorageBase::createRevision.
$this->assertTrue($node->hasTranslation('it'));
This feels like a massive change of expectations. If you revert to a revision before a translation a translation has been created I would not expect translations that were added afterwards to exist. I don't know but this choice seems deliberate.
Are we sure this change is correct?
Even though the minimum version of Redirect is now Drupal 10 I've made the change to isEmpty() as per @krystalcode's comment.
alexpott → changed the visibility of the branch 3451531-phpstan-issue-report to hidden.
Committed da29801 and pushed to 11.2.x. Thanks!
Committed and pushed 265731eb2b7 to 10.6.x and 392338a899e to 10.5.x. Thanks!
I did a cherry pick back to 10.x and fixed up the tiny conflict on commit.
Committed 6078270 and pushed to 11.x. Thanks!
We need an MR for 11.2.x - things have moved around a lot because hook stuff. ONce we have this in 11.2.x then yes we should get this in 10.6.x and 10.5.x too...
Given PHP has deprecated this it is no longer an issue for our coding standards committee (they have no choice here anymore) so merging this to 11.x to stop any new cases getting back in to core.
We need to open an issue to backport 📌 Inconsistent switch case syntax Downport to 10.6.x / 10.5.x so that branch can get PHP 8.5 testing and compatibility.
Committed ef77219 and pushed to 11.x. Thanks!
Looks great now. I dunno about formalising things - like this is the first XML fill we've added this to I think.
For me it bleongs belong the xml - it's got nought to do with the running of the file... and having it there would be inline with having it after the namespace like we do with PHP.
The aim is to have it easy to spot and easy to ignore.
Can we move it to the top of the file... because this can become a list of all the non-words in the file. It's easier to maintain I think.
More info on the .api.php changes... there are 3 hooks where this MR is remeving docs:
-
hook_redirect_load: the signature is wrong and this better documented by hook_ENTITY_TYPE_load() in entity.api.php -
hook_redirect_load_by_source_alter: does not exist anymore -
hook_redirect_prepare: does not exist anymore and is replaced by hook_ENTITY_TYPE_prepare_form (which is documented in entity.api.php)
The redirect.api.php stuff is changed because we've got PHPCS issues in the file
FILE: /Volumes/dev/sites/drupal8alt.dev/modules/redirect/redirect.api.php
-------------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 6 LINES
-------------------------------------------------------------------------
101 | ERROR | Missing parameter type
103 | ERROR | Missing parameter type
115 | ERROR | Missing parameter type
117 | ERROR | Missing parameter type
119 | ERROR | Missing parameter type
142 | ERROR | Missing parameter type
-------------------------------------------------------------------------
And it seems silly to fix things that are just not relevant anymore.
Re the PHPStan issues - I think we should address all of that in the follow-up - 🐛 PHPStan issue report in gitlab Active
Let's not add this to the dictionary - it is not a word.
We can added it to the phpcs.xml.dist file like so:
<?xml version="1.0" encoding="UTF-8"?>
<!-- cspell:ignore Openercase -->
<ruleset name="drupal_core">
Committed and pushed 967e3af639f to 11.x and d6d4d9cd564 to 11.2.x. Thanks!
I've searched the code for any conditions on drupal's version or PHP version and have fixed what I found.
@berdir yay! No regressions :)
We need to land 📌 Drop Drupal 9 and PHP 7 support Active first.
@krystalcode great point about the Drupal 9 support - agreed with @Berdir that we're going to drop that in 📌 Drop Drupal 9 and PHP 7 support Active - dropping Drupal 9 support doesn't break their sites FWIW.