Netherlands
Account created on 8 December 2015, over 9 years ago
#

Merge Requests

More

Recent comments

🇳🇱Netherlands bbrala Netherlands

Added the changes from the parent, lets get some working tests.

🇳🇱Netherlands bbrala Netherlands

Cherry picked the change, we need to make tests.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

Although there is some missing test:

  1. The constraint MatchesOtherConfigValue needs tests
  2. The constraint NoEntitiesExistYetWithHigherCardinality needs tests

Our tests are green :x

🇳🇱Netherlands bbrala Netherlands

Rebased, lets see if this is still green.

🇳🇱Netherlands bbrala Netherlands

bbrala made their first commit to this issue’s fork.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

Conflict was on cspell word list. Rebased, keeping rtbc

🇳🇱Netherlands bbrala Netherlands

Seems nicely rebased, back to RTBC

🇳🇱Netherlands bbrala Netherlands

There is agreement on closing this, as per docing standars meeting feb 25th

🇳🇱Netherlands bbrala Netherlands

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?

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

Merged 11.x into this, lets see what happens.

🇳🇱Netherlands bbrala Netherlands

bbrala made their first commit to this issue’s fork.

🇳🇱Netherlands bbrala Netherlands

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

🇳🇱Netherlands bbrala Netherlands

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
🇳🇱Netherlands bbrala Netherlands

Still some test errors left in biuld/jsonapi and randomjs kinda test.

🇳🇱Netherlands bbrala Netherlands

Seems a test is still fialing since it expects a string, not null.

🇳🇱Netherlands bbrala Netherlands

Commentable makes so much sense i dont want to change it, just added to the drupal words. THink this can go back to NR

🇳🇱Netherlands bbrala Netherlands

Next up, this needs a little word making sure the fixture/config* is updated to reflect some of the config changes.

🇳🇱Netherlands bbrala Netherlands

bbrala made their first commit to this issue’s fork.

🇳🇱Netherlands bbrala Netherlands

bbrala made their first commit to this issue’s fork.

🇳🇱Netherlands bbrala Netherlands

Well at least tests are close to OK again so this can be worked on again.

🇳🇱Netherlands bbrala Netherlands

As per Alex, we need a comment there, i understand it feels fragile, unfortunately a result of how things are run.

🇳🇱Netherlands bbrala Netherlands

Todo: re-addthe creation of the entity types removed in one of last 2 commits

🇳🇱Netherlands bbrala Netherlands

Seems good enough, moving to main MR, shoudln't need 'Needs review' :)

🇳🇱Netherlands bbrala Netherlands

Scary rebase, so made a new branch first to see how it handles itself.

🇳🇱Netherlands bbrala Netherlands

Seems fixtures might need work. Not sure how those work.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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

🇳🇱Netherlands bbrala Netherlands

Perhaps this will help getting overviews for such things: Support glob style filter in filter-keys Active

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

As we have support from @catch @smustgrave @bbrala and @_nod i think its save to say this is RTBC.

🇳🇱Netherlands bbrala Netherlands

Good to have the c9verage. +1.

Maybe make a mr with the change to maintainers.txt?

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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)

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

Lets recap current state.

  1. #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.
  2. EntityValidationTrait.php:111: @todo Add recursion … or add a helper to TypedConfigManager?
  3. 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.
🇳🇱Netherlands bbrala Netherlands

Test failure is unrealated. I feel this might actually be ready? o_O

🇳🇱Netherlands bbrala Netherlands

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

🇳🇱Netherlands bbrala Netherlands

Crediting casey for helping with the config type definition puzzle.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

Fair enough. Looking at the change, this seems like a good update to make sure we actually test the correct key.

🇳🇱Netherlands bbrala Netherlands

This issue is now contained in the parent issue.

🇳🇱Netherlands bbrala Netherlands

Weird. Was sure to pull 11.x and accept theirs in git.

🇳🇱Netherlands bbrala Netherlands

Did a quick rebase, since it is only phpstan baseline, setting to RTBC

🇳🇱Netherlands bbrala Netherlands

Lets try again, updated to 11.x and fixed tests <3

🇳🇱Netherlands bbrala Netherlands

bbrala changed the visibility of the branch 3104408-allow-to-include to hidden.

🇳🇱Netherlands bbrala Netherlands

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 :)

🇳🇱Netherlands bbrala Netherlands

As an artifact on gitlab :)

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

The rebase onto 11.0.x was no fun, lets see how it fares.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

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.

🇳🇱Netherlands bbrala Netherlands

Won't it make sense to wait for the maintainership here? I don't think we can go much faster than they are.

🇳🇱Netherlands bbrala Netherlands

Maintainer has een contacted through slack and Drupal contact form.

🇳🇱Netherlands bbrala Netherlands
class TemporaryQueryGuard {

  public const JSONAPI_FILTER_AMONG_ENABLED = 'filter_among_enabled';

...
}

That's the class constant route i talked about earlier.

🇳🇱Netherlands bbrala Netherlands

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.

Production build 0.71.5 2024