- 🇨🇦Canada joseph.olstad
Going through important patches that we have been using for several years. It would be nice to get this one in.
All the nits from #89 are resolved by 100 and then 102
See interdiff.
- Status changed to Needs review
almost 2 years ago 1:16am 15 February 2023 - Status changed to RTBC
almost 2 years ago 5:45am 15 February 2023 - 🇨🇦Canada joseph.olstad
Justification for RTBC:
- #64 tests only proves the fix is needed.
- Nits resolved from #89
- Patch 102 passes all tests on 9.4.x
- Patch 102 passes all tests on 9.5.x
- Patch 102 passes all tests on 10.0.x
- Patch 102 passes all tests on 10.1.x
- Status changed to Needs review
almost 2 years ago 6:47am 15 February 2023 - 🇨🇳China jungle Chongqing, China
> I agree with @berdir that adding this @internal method to the interface is a BC break. Please remove the method from the interface.
-- from #89.4
+++ b/core/lib/Drupal/Core/TypedData/TranslatableInterface.php @@ -30,6 +30,18 @@ interface TranslatableInterface { + public function setIsDefaultTranslation(bool $is_default_translation = NULL);
+1 to remove this.
Otherwise, RTBC +1
The last submitted patch, 106: 2810355-106-test-only.patch, failed testing. View results →
- 🇨🇳China jungle Chongqing, China
Tagging "Needs Review Queue Initiative" looks for review.
- Status changed to Needs work
almost 2 years ago 10:04am 15 February 2023 - 🇬🇧United Kingdom catch
I don't think #106 addresses the bc break brought up by @berdir.
+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php @@ -947,7 +947,16 @@ protected function invokeFieldMethod($method, ContentEntityInterface $entity) { $original_langcodes = array_keys($entity->original->getTranslationLanguages()); foreach (array_diff($original_langcodes, $langcodes) as $removed_langcode) { + /** @var \Drupal\Core\Entity\ContentEntityInterface $translation */ $translation = $entity->original->getTranslation($removed_langcode); + + // Fields may rely on the isDefaultTranslation() method to determine + // what is going to be deleted - the whole entity or a particular + // translation. + if ($translation->isDefaultTranslation()) { + $translation->setIsDefaultTranslation(FALSE); + } +
If we don't check an interface (or ContentEntityBase), we can't rely on it here - instead of an immediate fatal error because you haven't implemented the method, you'd get one when this code runs, which would be a slower but nastier surprise.
I think this probably needs a new interface, which ContentEntityBase then implements, and then we can check instanceof for that interface.
EntityTranslatableInterface maybe?
Then we could open a follow-up merge that back into TranslatableInterface for 11.0.0
- 🇨🇭Switzerland berdir Switzerland
+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php @@ -947,7 +947,16 @@ protected function invokeFieldMethod($method, ContentEntityInterface $entity) { + /** @var \Drupal\Core\Entity\ContentEntityInterface $translation */ $translation = $entity->original->getTranslation($removed_langcode); + + // Fields may rely on the isDefaultTranslation() method to determine + // what is going to be deleted - the whole entity or a particular + // translation. + if ($translation->isDefaultTranslation()) { + $translation->setIsDefaultTranslation(FALSE); + } + $fields = $translation->getTranslatableFields(); foreach ($fields as $name => $items) {
If it's a temporary name then I'd go with ha specific name for the interface, like SettableDefaultTranslationInterface.
But then we also need to do a deprecation messsage if an entity doesn't implement that and the bug won't be fixed there for now.
And then in D11, we need to deprecate the interface and remove it in D12 :-/.
I do wonder if we can somehow find a different solution for this.
I think it would kind of make sense if invokeFieldMethod() would be on the entity, then it would be an internal property that we could handle without a new public method for one very specific use case. But moving that has exactly the same problem.
The other idea is that we're not actually deleting a translation, we're changing the langcode of an existing one. Feels weird to call delete at all in this scenario, but we're doing it to counter the equally "wrong" is-new-translation check in \Drupal\file\Plugin\Field\FieldType\FileFieldItemList::postSave(), so we add a usage and then remove one again, resulting in no change (with the patch).
I don't have a completely formed idea, but I'll try to think about it a bit more.
- 🇨🇭Switzerland berdir Switzerland
What about just doing a method_exists() check on $entity for BC?
The problem that I see is that we can't detect this at runtime in a useful way. We could easily add an else to this logic and then trigger the deprecation message, but the chances of an actually working full implementation of ContentEnttityInterface that actually get saved are near-zero. The cases we're working with are edge cases that aren't really entities. Searching a bit, I couldn't find any examples even for that, I suspect simple_oauth has been refactored to no longer do that.
That means that if there really are such classes, they will never get in there, they are most likely also not actual entity types, so we have no easy mechanism to find them and trigger a deprecation message. We could maybe do something with phpstan or so, I checked if symfony/deubgclassloader has anything for that, but I think not.
So, method_exists(), deprecation message, and worst case is someone will get an easy to fix fatal error on D11 when they update.
- Status changed to Needs review
over 1 year ago 1:32pm 21 July 2023 - last update
over 1 year ago 29,838 pass, 2 fail - 🇨🇭Switzerland berdir Switzerland
Here's a patch for that, also created a change record. I added a @trigger_error, while I don't expect this will ever be triggered, it also serves as a reminder to make that change in D11.
The last submitted patch, 113: 2810355-113.patch, failed testing. View results →
- last update
over 1 year ago 29,841 pass - 🇨🇦Canada joseph.olstad
Thanks for that @Berdir .
This is a pretty important fix, I hope that the core maintainers prioritize this. The current behavior was a nasty surprise when I relied on file usage and ended up losing a lot of media due to the bug.
- 🇺🇸United States smustgrave
Removing CR tag as I see that was added.
Can the needs subsystem maintainer tag be removed since @berdir, @catch, and @hchonov are all on this ticket?
Leaving in NR but can deprecation message be updated in #115 also please.
- 🇨🇦Canada joseph.olstad
@smustgrave, what part of the deprecation message needs changing?
In #115 it is as follows:
@trigger_error('Not providing a setIsDefaultTranslation() method when implementing \Drupal\Core\TypedData\TranslatableInterface is deprecated in drupal:9.4.0 and is required from drupal:11.0.0. See https://www.drupal.org/node/3376146', E_USER_DEPRECATED);
- 🇨🇭Switzerland berdir Switzerland
The version, it needs to say 10.2 instead of 9.4. We can't retroactively deprecate it, this will not be backported as it introduces a new method.
- Status changed to Needs work
over 1 year ago 2:24pm 31 July 2023 - 🇺🇸United States smustgrave
Brought up in #needs-review-queue-initative and can remove the needs subsystem maintainer tag.
Moving to NW for the trigger_error update so that doesn't get lost.
- Status changed to Needs review
over 1 year ago 8:46pm 31 July 2023 - last update
over 1 year ago 29,912 pass - 🇨🇦Canada joseph.olstad
Ok thanks for clarifying, here's a new patch and interdiff.
This patch applies on 10.0.x 10.1.x and 11.x
- last update
over 1 year ago 29,457 pass - last update
over 1 year ago 28,527 pass - last update
over 1 year ago 30,342 pass - Status changed to RTBC
over 1 year ago 2:03pm 1 August 2023 - 🇺🇸United States smustgrave
Thanks!
Going to mark this and see if we can get into 10.2 soon!
- last update
over 1 year ago 29,947 pass - last update
over 1 year ago 29,954 pass - last update
over 1 year ago 29,954 pass - last update
over 1 year ago 29,959 pass 0:41 57:13 Running- last update
over 1 year ago 29,959 pass - last update
over 1 year ago 29,960 pass - last update
over 1 year ago 29,968 pass - 🇩🇪Germany Anybody Porta Westfalica
Thank you all very very much. We just ran into this in a Drupal 10.1.2 project and I was searching for hours until I found the solution here. Indeed critical and super nice to see this fixed, thank you so much! Confirming that #121 fixed the issue.
I first thought it was 🐛 Private file download returns access denied, when file attached to revision other than current Needs work .
Would be really important to have this fixed in the next possible release.
- last update
over 1 year ago 30,050 pass - last update
over 1 year ago 30,057 pass - last update
over 1 year ago 30,045 pass, 2 fail The last submitted patch, 121: 2810355-121.patch, failed testing. View results →
- last update
over 1 year ago 30,061 pass - 🇫🇮Finland lauriii Finland
Random fail in
Drupal\Tests\Component\Utility\RandomTest
. - last update
over 1 year ago 30,064 pass - last update
over 1 year ago 30,131 pass - last update
over 1 year ago 30,136 pass - last update
over 1 year ago 30,137 pass - last update
over 1 year ago 30,137 pass - last update
over 1 year ago 30,147 pass - last update
over 1 year ago 30,147 pass - last update
over 1 year ago 30,149 pass - last update
over 1 year ago 30,155 pass - last update
over 1 year ago 30,162 pass - last update
over 1 year ago 30,168 pass - last update
over 1 year ago 30,169 pass - last update
over 1 year ago 30,206 pass - last update
over 1 year ago 30,206 pass 45:42 42:04 Running- Status changed to Needs work
about 1 year ago 12:27pm 28 September 2023 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
-
+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php @@ -86,6 +86,18 @@ abstract class ContentEntityBase extends EntityBase implements \IteratorAggregat + protected $isDefaultTranslation = NULL;
I think this should be
protected bool|null $enforceDefaultTranslation = NULL;
As this is consistent with $enforceRevisionTranslationAffected.
-
+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php @@ -409,10 +421,26 @@ public function setRevisionTranslationAffectedEnforced($enforced) { + public function setIsDefaultTranslation(bool $is_default_translation = NULL) {
If we look at the methods above this I think this method naming is awkward...
The code above this is:
/** * {@inheritdoc} */ public function isRevisionTranslationAffectedEnforced() { return !empty($this->enforceRevisionTranslationAffected[$this->activeLangcode]); } /** * {@inheritdoc} */ public function setRevisionTranslationAffectedEnforced($enforced) { $this->enforceRevisionTranslationAffected[$this->activeLangcode] = $enforced; return $this; }
I also think we should not provide the default for the argument.
Should have a return type of :static and the @return needs documenting.
So I this should be
public function setDefaultTranslation(bool|null $is_default_translation): static {
-
- Status changed to Needs review
about 1 year ago 5:11am 17 October 2023 - last update
about 1 year ago 30,414 pass - 🇺🇸United States benjifisher Boston area
@alexpott:
- Are we supposed to declare the type of any new class property? If so, I think we should use
?bool
instead ofbool|null
. I like the idea of naming the new propertyenforce...
. - For consistency, I think the function name should end with
Enforced
. Again, I think we should use?bool
.
I am also updating some of the comments.
Personally, I would use
public function isDefaultTranslation() { return $this->enforceDefaultTranslation ?? $this->activeLangcode === LanguageInterface::LANGCODE_DEFAULT; }
but I will restrain myself since this is someone else's code.
- Are we supposed to declare the type of any new class property? If so, I think we should use
- Status changed to RTBC
about 1 year ago 2:17pm 17 October 2023 - Status changed to Fixed
about 1 year ago 4:22pm 17 October 2023 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
Committed and pushed aa3ecb2c2d2 to 11.x and 4976f3a460f to 10.2.x. Thanks!
-
alexpott →
committed aa3ecb2c on 11.x
Issue #2810355 by joseph.olstad, Leksat, ravi.shankar, Berdir, jungle,...
-
alexpott →
committed aa3ecb2c on 11.x
-
alexpott →
committed 4976f3a4 on 10.2.x
Issue #2810355 by joseph.olstad, Leksat, ravi.shankar, Berdir, jungle,...
-
alexpott →
committed 4976f3a4 on 10.2.x
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
I created 📌 Add setDefaultTranslationEnforced to \Drupal\Core\TypedData\TranslatableInterface Active so we remember to add the method to the interface.
Automatically closed - issue fixed for 2 weeks with no activity.