Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Sometimes developers will have invalid YAML files. In my case, I botched a .routing.yml file, but edited many more. Commands like drush cr
then correctly say "Unable to parse at line xyz", but not which file.
Committed 45f97bc and pushed to 8.8.x. Thanks!
Comment | File | Size | Author |
---|---|---|---|
#20 | interdiff.3046298.15-20.txt | 2.76 KB | Gogowitsch |
#20 | 3046298-20.drupal.DX-tell-developer-which-yaml-file-caused-a-ParseException.patch | 5.82 KB | Gogowitsch |
#16 | interdiff-3046298-13-16.txt | 476 bytes | voleger |
Comments
Comment #2
Gogowitsch CreditAttribution: Gogowitsch as a volunteer and at QuoData GmbH Quality & Statistics for Merck KGaA commentedAdded patch.
Comment #3
Gogowitsch CreditAttribution: Gogowitsch as a volunteer and at QuoData GmbH Quality & Statistics for Merck KGaA commentedTo test this, I used a
my_module_name.routing.yml
file and added a row with just a single:
on it.Comment #4
jhedstromThis looks like a great DX win!
I think a few quick unit test cases could be added to the existing
\Drupal\Tests\Component\Discovery\YamleDiscoveryTest
to ensure the expected exception messages are thrown for the 2 added exceptions.Comment #5
jhedstromSetting to NW for tests.
Comment #6
Gogowitsch CreditAttribution: Gogowitsch as a volunteer and at QuoData GmbH Quality & Statistics for ÖQUASTA commentedThanks for the nudge. I have added a test to see if the expected exception is thrown and if it contains the filename.
Now I just need to find the upload button for my new patch - why is the d.o UI different today?
Comment #7
Gogowitsch CreditAttribution: Gogowitsch as a volunteer and at QuoData GmbH Quality & Statistics for ÖQUASTA commentedAh, the upload is not offered for the first comment directly after logging in. Sorry for the noise, patch is attached.
Comment #9
jhedstromThe test is failing because this is using a class from core in a component:
There is also a
YamlDiscovery
class in the component namespace, so if you switch to using that instead, the tests should go green.Comment #10
Gogowitsch CreditAttribution: Gogowitsch as a volunteer and at QuoData GmbH Quality & Statistics for ÖQUASTA commentedChanges since the last patch:
Component/Discovery/YamlDiscoveryTest.php
except for alphabetic use statementsCore
instead of theComponent
namespaceComment #11
Gogowitsch CreditAttribution: Gogowitsch as a volunteer and at QuoData GmbH Quality & Statistics for ÖQUASTA commentedTests pass, so this issue now "Needs review".
Comment #12
jhedstromThe issue with Core vs Component test issue got me digging into the Component implementation and I realized it has the same DX issue.
I think this bit of logic should also go into
\Drupal\Component\Discovery\YamlDiscovery::decode()
, and then your previous test for the component can be added back, and we'll have a test for both core and component versions, and fix the DX issue in both places.Sorry I didn't catch that in a previous round of feedback.
The rest looks good to go!
Comment #13
Gogowitsch CreditAttribution: Gogowitsch as a volunteer and at QuoData GmbH Quality & Statistics for ÖQUASTA commentedYeah, I agree. Thanks for your review!
In comparison to the last patch, the new one fixes the DX issue also on the Component side.
Comment #14
jhedstromThis looks great! Thanks for sticking with all the reviews :)
Comment #16
volegerJust CS fix, back to RTBC
Comment #18
Gogowitsch CreditAttribution: Gogowitsch as a volunteer and at QuoData GmbH Quality & Statistics for ÖQUASTA commentedThe patch from #16 looks good to me.
Tests are green now. As I see no relationship between our changes here and
LayoutBuilderDisableInteractionsTest
, I attest the previous failure to a random flakiness. Other opinions are welcome.Comment #19
larowlanThere doesn't appear to be any test coverage for this, only for the invalid data type exception.
we should be adding that here if we want this feature to stick around
Other than that, this is a great feature, would save so much pain - thanks!
Comment #20
Gogowitsch CreditAttribution: Gogowitsch as a volunteer and at QuoData GmbH Quality & Statistics for ÖQUASTA commentedThere is currently no
Yaml::*
call within Drupal that will throw aParseException
. The Symfony component solely throwsParseExceptions
, and the 2 existing Drupal wrappers in core solely throwInvalidDataTypeExceptions
.Changes since the last patch:
SerializationInterface
: documented thatencode
anddecode
may throw anInvalidDataTypeException
.Comment #21
alexpottThis is line with other parts of the system for example - \Drupal\Core\Asset\LibraryDiscoveryParser::parseLibraryInfo() and looks great. Nice work.
https://www.drupal.org/files/issues/2019-06-05/3046298-20.drupal.DX-tell...