- Issue created by @mondrake
- Status changed to Needs review
5 months ago 12:08pm 2 April 2024 - Status changed to Needs work
5 months ago 1:36pm 2 April 2024 - 🇺🇸United States smustgrave
Left a comment but appears lost test coverage right? Let me know if I'm wrong.
- Status changed to Needs review
5 months ago 1:46pm 2 April 2024 - 🇮🇹Italy mondrake 🇮🇹
I found the issue I mentioned inline, it's ✨ Allow kernel tests to fail or expect logged errors Needs work .
- Assigned to mondrake
- Status changed to Needs work
5 months ago 4:47pm 2 April 2024 - 🇮🇹Italy mondrake 🇮🇹
Maybe we can do a test, using a prophecy, somehow. Will try.
- Issue was unassigned.
- Status changed to Needs review
5 months ago 7:47pm 2 April 2024 - Status changed to RTBC
5 months ago 9:48pm 2 April 2024 - First commit to issue fork.
- 🇬🇧United Kingdom catch
@alexpott asked me about bc concerns given constructor property promotion here means we now have two properties that were previously untyped getting a type. The constructor type doesn't actually change in this issue because that was already type hinted, it's only the property itself.
Properties are considered internal unless they're on a base class per https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... →
We sometimes/often deprecate properties before we remove them, but we don't have that many cases of adding types to existing properties. However, I think this is fine - someone would have to be setting the property to a value with the wrong type, or overriding the property with the wrong type. So this is both covered by the bc/internal policy and also seems very unlikely to actually break anything - even if someone is subclassing this class it would be unlikely that they'd be affected by the change.
I removed the constructor docs on commit.
Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!
- Status changed to Fixed
5 months ago 10:58am 3 April 2024 - 🇬🇧United Kingdom longwave UK
// For a typed config the data MUST exist.
If it MUST exist should this be an exception? or at least an assert()?
-
alexpott →
committed 1d03e28a on 10.3.x
Revert "Issue #3437566 by mondrake, smustgrave: Replace trigger_error in...
-
alexpott →
committed 1d03e28a on 10.3.x
-
alexpott →
committed f7cb9cfb on 11.x
Revert "Issue #3437566 by mondrake, smustgrave: Replace trigger_error in...
-
alexpott →
committed f7cb9cfb on 11.x
- Status changed to Needs work
5 months ago 2:07pm 3 April 2024 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
@longwave pointed out that we should thrown an \ InvalidArgumentException here. We replaced a non-recoverable error with a log. That's not right...
- Status changed to Needs review
5 months ago 3:12pm 3 April 2024 - Status changed to RTBC
5 months ago 3:28pm 3 April 2024 - 🇺🇸United States smustgrave
Exception appears to be added correctly after the log.
- Status changed to Needs work
5 months ago 3:35pm 3 April 2024 - 🇬🇧United Kingdom longwave UK
Needs a new title and IS update given the new approach.
- Status changed to RTBC
5 months ago 4:23pm 3 April 2024 - Status changed to Needs work
5 months ago 4:37pm 3 April 2024 - 🇬🇧United Kingdom longwave UK
I think we can remove the logger again - the exception is enough.
- Status changed to Needs review
5 months ago 10:12pm 11 April 2024 - Status changed to RTBC
5 months ago 1:43pm 12 April 2024 - Status changed to Fixed
5 months ago 9:37am 14 April 2024 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
Committed and pushed ea5932e490 to 11.x and 29964e3a3a to 10.3.x. Thanks!
-
alexpott →
committed 29964e3a on 10.3.x
Issue #3437566 by mondrake, longwave, catch, smustgrave, alexpott:...
-
alexpott →
committed 29964e3a on 10.3.x
-
alexpott →
committed ea5932e4 on 11.x
Issue #3437566 by mondrake, longwave, catch, smustgrave, alexpott:...
-
alexpott →
committed ea5932e4 on 11.x
Automatically closed - issue fixed for 2 weeks with no activity.