I've split two issues for the missing tests.
Added the changes from the parent, lets get some working tests.
Cherry picked the change, we need to make tests.
Fun fact is that indexes have a default, like value => [value], but that the shape can be defined in the childs, therefor this is really hard to reallt validate.
Although there is some missing test:
- The constraint MatchesOtherConfigValue needs tests
- The constraint NoEntitiesExistYetWithHigherCardinality needs tests
Our tests are green :x
Rebased, lets see if this is still green.
bbrala → created an issue.
bbrala → created an issue.
I would advise we'd also add an optinal argument limit. So you can fetch x of the overview and perhaps make sure people dont start using this and end up querieing thousands of nodes.
This seems good, went through logs and didnt really notice anything wrong. We (castch, mstrelan) have discussed how one would run the child pipeline with failing unit tests, for now manually adding allow failure to the unit tests will be fine. Setting to RTBC.
Conflict was on cspell word list. Rebased, keeping rtbc
Seems nicely rebased, back to RTBC
There is agreement on closing this, as per docing standars meeting feb 25th
I'll try and dive into this sometime soon.
One thing that I did notice, there were some conceirns in #77, are those valid or handled?
All green.
Did some digging. Ended up removing the exception and checking for null and '' in IsCommentable and EntityTypeExists. Seems to make more sense. Also adjusted the test so it expects extra validation errors when looking for null.
Well, 2 test failures:
1. core/recipes/feedback_contact_form/recipe.yml -> The receipient input is not set when installing in core/tests/Drupal/Tests/Core/Recipe/RecipeQuickStartTest.php, so it fails since ${resipient} is not a vaild email adress.
2. core/tests/Drupal/FunctionalTests/Core/Recipe/StandardRecipeInstallTest.phphas the same issue, no recipient set.
Merged 11.x into this, lets see what happens.
Ok, so the issue remaining is the fact that in ConfigEntityValidationTestBase::testRequiredPropertyValuesMissing it tries to test what happens when a property is set to null. When EntityTypeExistsConstraintValidator is validated it also executes the code:
if (!is_string($value)) {
throw new UnexpectedTypeException($value, 'string');
}
Which is then thrown. When looking at other implementations where this eror is thrown i see ContentLanguageSettings with EntityBundleExists. This sems to be an immutable property.
I'm now guessing there is 2 possible solutions:
1. The target_entity_type_id property of the comment type should be immutable.
2. It is not immutable, but instead of throwing an UnexpectedTypeException is might just need to add a validation error that the property shhould be string.
What the right solution is, i dont know. Hopefully someone does :x
I don't get why the id cannot be string and this test fails. Trying to figure why that would not be allowed. The schema does tell us string is fine, is that wrong then?
Drupal\Tests\comment\Kernel\CommentStringIdEntitiesTest::testCommentFieldNonStringId
Failed asserting that exception of type "Drupal\Core\Config\Schema\SchemaIncompleteException" matches expected exception "UnexpectedValueException". Message was: "Schema errors for comment.type.foo with the following errors: 0 [target_entity_type_id] The 'entity_test_string_id' entity is not commentable" at
core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:98
vendor/symfony/event-dispatcher/EventDispatcher.php:206
vendor/symfony/event-dispatcher/EventDispatcher.php:56
core/lib/Drupal/Core/Config/Config.php:230
core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:260
core/lib/Drupal/Core/Entity/EntityStorageBase.php:487
core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:239
core/lib/Drupal/Core/Entity/EntityBase.php:370
core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:618
core/modules/comment/tests/src/Kernel/CommentStringIdEntitiesTest.php:53
Still some test errors left in biuld/jsonapi and randomjs kinda test.
Seems a test is still fialing since it expects a string, not null.
Commentable makes so much sense i dont want to change it, just added to the drupal words. THink this can go back to NR
Next up, this needs a little word making sure the fixture/config* is updated to reflect some of the config changes.
Well at least tests are close to OK again so this can be worked on again.
As per Alex, we need a comment there, i understand it feels fragile, unfortunately a result of how things are run.
Todo: re-addthe creation of the entity types removed in one of last 2 commits
Seems good enough, moving to main MR, shoudln't need 'Needs review' :)
Scary rebase, so made a new branch first to see how it handles itself.
bbrala → made their first commit to this issue’s fork.
Seems fixtures might need work. Not sure how those work.
Removing wim from assignment, that will probably not make this go faster.
My conclusion in #27 is that null is a good idea. I rebased and if tests don't fail I think this can move forward as is. Since my additions were minimal, setting RTBC.
Going through the config issues a bit to see where things are hanging.
I read through all this, and have not seen any discussion around the NULL change. I understand the text in the CR ("The reason is because an empty string makes no sense for this field. "" is never a useful description of a form mode."). But I dont really see why this is as much better as it is a change.
If i try to argue why it should be null: Now i want no desription is equal to en empty description. This unfortunately meant he field is not really optional, when you wouldn't post an description you technically tend a NULL, which cannot be stored right now. But if we allow null, we would be able to save without the field, since null is fine.
This could have reasons in jsonapi, maybe we can then post without the field and make it null, instead of requiring an empty string?
This is as far as i get.
Added a test for the validation. Removed an extra comment and added possible follow up ✨ [Follow-up] action.configuration.user_remove_role_action could also use a UserHasRole constaint Active
bbrala → created an issue.
Perhaps this will help getting overviews for such things: ✨ Support glob style filter in filter-keys Active
Ty!
For jsonapi to accept posting/patching this, it needs to be flagged as fullyvalidatable. That is how i got here, trying to enable system.action tests in jsonapi.
I saw the test you mentioned, I'll use that as a base. Just wanted to validate me understanding how to do this.
As we have support from @catch @smustgrave @bbrala and @_nod i think its save to say this is RTBC.
Good to have the c9verage. +1.
Maybe make a mr with the change to maintainers.txt?
Fixed in parent issue.
#2905594: Missing entity validation constraint: don't allow new entities when there is an existing one with the same ID → is no longer needed, added a check for existing config entities by id (and fallback to uuid) to make sure they don't exist.
When working on
✨
[PP-2] POST/PATCH config entities via REST for config entity types that support validation
Needs work
i did notice after actually enabling tests for all validatable config entities, that although system.action.* is validated, the plugins aren't. So when testing, using user_unblock_user_action it errors on that fact.
Not sure if would have marked action as validatable when the configuration you might be able to put in there (from core) is not.
Ok, tests are now properly failing on ConfigurationEntitites that are either missing a postDocument or those who are validatiable but have plugins that are not (see failing ActionTest)
Hmm, also see some other thing that need work. Not sure the removal of the SKIP_TESTS is doing what it should.
I'll get back to this.
Lets recap current state.
- #2905594: Missing entity validation constraint: don't allow new entities when there is an existing one with the same ID → is still mentioned in the IS as something to investigate.
EntityValidationTrait.php:111: @todo Add recursion … or add a helper to TypedConfigManager?ResourceTestBase:2086: 2086: @todo Config schema assumes at least high-level type compliance, which is violated here. Harden it to provide equivalently helpful error responses.
Test failure is unrealated. I feel this might actually be ready? o_O
I've updated the changerecord, went through credits. Also went through open credits and it seems all is addressed.
Now i need to investigate testing failures ;x
Crediting casey for helping with the config type definition puzzle.
Rebased, did need to change a few lines to adjust to return types and the changes to the base class (doTestPatchIndividual). Lets see what it tells us.
I'll update this MR and the comments and such.
I think this is good, and helpful to get the last bit out of the module file. Although there are perhaps ways to improve this class even more, this would mean more changes, and bigger impact. This is still a temporary (ha! years) class. Lets isolate this to the existing classes and change as little as possible to make the change possible.
Yeah, i dont think you need test coverage there, you do need to address the other small nits though. Although the out of scope space for TRUE i'd say is ok.
Fair enough. Looking at the change, this seems like a good update to make sure we actually test the correct key.
heroicnick → credited bbrala → .
TIL will be more carefully with phostan diffs
This issue is now contained in the parent issue.
Weird. Was sure to pull 11.x and accept theirs in git.
Did a quick rebase, since it is only phpstan baseline, setting to RTBC
Lets try again, updated to 11.x and fixed tests <3
bbrala → changed the visibility of the branch 3104408-allow-to-include to hidden.
I actually ran into this issue earlier today by chance (the paths not being correct).
This looks good and will make some things easier, but also allow for a good interface in gitlab for changes as per 📌 [CI] Report PHPStan baseline statistics in job Active .
All good, working as intended :)
As an artifact on gitlab :)
I think i mentioned that earlier. It uses the stored metrics from the target branch of the mr as a base and shows the diff compared to that.
I've checked the reporting thingie, it seems reports are always uploaded so my assumption (and therefor the use of a follow up) is invalid. I'm going to update the mr here to include the metrics for easy viewing.
The rebase onto 11.0.x was no fun, lets see how it fares.
Reviewed the code. This looks good, changes are all compliant with the original hooks. And lovely to slowly see module files die. ;)
+1 to RTBC from me.
Did a. test here: https://git.drupalcode.org/project/deprecated_code/-/merge_requests with a mr to main and 1.x with different metrics.
Few things to note:
Metric changes are based on the target branch of the merge request, which means this will work pretty quite well to show the correct metrics based on the main branches (11.x, 11.1.x etc) without the need of any weirdness or logic.
Seems also to be provable with a MR locally, but since the current job only submits artifacts when the job fails it might need a little more massaging. So lets follow up on that after this issue.
And an example:
https://gitlab.com/gitlab-org/ci-sample-projects/metrics-reports
File format is extremely easy
No wait, shouldn't we expose this as a metrics report?
https://docs.gitlab.com/ee/ci/testing/metrics_reports.html
This would make this visible in the gitlab interface.
Won't it make sense to wait for the maintainership here? I don't think we can go much faster than they are.
Maintainer has een contacted through slack and Drupal contact form.
class TemporaryQueryGuard {
public const JSONAPI_FILTER_AMONG_ENABLED = 'filter_among_enabled';
...
}
That's the class constant route i talked about earlier.
The notorius temporary class.
I kinda agree that the whole enum dance doesn't really add much. If we are to change this a value object could make sense, or, since this is really only used in this class, perhaps just have class constants with limited visibility? Then we also scope it into this class where it is used, and then can clean up the .module file?
It is used in contrib though: https://git.drupalcode.org/search?group_id=2&scope=blobs&search=-path%3A...
so Limited visibility might not be a good idea.