πŸ‡ΊπŸ‡ΈUnited States @neclimdul

Houston, TX
Account created on 7 February 2006, about 18 years ago
  • Senior Drupal Architect at APQCΒ 
#

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Updated IS

Since the changes should only affect changing I don't think this counts as an "API Change" so captured that in the IS.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

I'm not sure that the sync is required but i don't see a problem with it so here's a combined version with the sync and also the more robust helper method.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

"I would agree with the overall goal that Recipes are currently progressing how config works in drupal core, providing tools 'more like' Features in Drupal7 ecosystem."

Not sure what that means. For my use cases there are better versions of everything in the D8+ ecosystem so I'm just not the target audience.

"To mention the 'want's of this issue that I would ask around that recipes isn't handling (that i know of!). is ENV based config (config_splits using its 'partial configs') dev/uat/prod codes. Otherwise the goal of having better code side tools and UI's are part of the Recipes outlines.."

"Yes, I agree that ENV based config should be easier. Currently, it makes more sense to use settings.php to override config, because then I don't have to manage multiple partial configs."

I use environment driven config _very_ heavily and its pretty straightforward today. Keys module is great if a little rough in the UI. Its designed for "secrets" but the model is allowing the mapping from stores to any config value so mapping from environment to any config value is easy and very powerful and has so far met every need I've had for varying settings per environment with the exception of things like enabling development features locally which still make since to manage with splits.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

I'm not sure. Looking through the summary, it seems like this was about taking a fresh look at config and taking the underlying API to the next level. The initiative you linked seems to be about providing better starter configuration/profiles/recipes. I could see how updating the config API could help that but those sound like different things and the issues tagged to CMI 2.0 don't seem particularly tied to that initiatives goals.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

neclimdul β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Made a library people can use to "test" this in their projects.

https://www.drupal.org/project/cache_remember β†’
https://packagist.org/packages/drupal/cache_remember

I'm fairly happy with the current version of the methods with one exception and that's the fact its still annoying to set the expiry since we set a timestamp instead of an offset. You need to pull the time service and calculate it instead of having a central way of doing this.

Not sure if there's a good way to address this while maintaining consistency with other Drupal API's but I'm open to suggestions and we can test them out.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Do we need to hang this on a ckeditor feature with an indeterminate timeline? Seems like we could provide this as a feature with the caveat and support the new feature after ckeditor supports. That would provide some immediate functionality for people that can use it as is but also highlight the feature request to more users.

With that in mind though, we should probably default this off and make sure administrators opt in and understand what they're opting in to.

A quick test of the merge request, this seems to work pretty well as a user. Very nice!

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Is the proposed resolution that was agreed to the static list?

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Moving parent since we dropped the deprecation path.

Thought this had stalled but looks like a lot of great work was happening I just wasn't watching the right issues! Thanks!

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

"as long as we're using the correct type of merge pipeline." welp...

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Rebased and address the feedback from alex.

Unfortunately this doesn't work. It was really broken and the ID attachement didn't seem to work at all. I fixed that but it only works for simple elements. When trying to attach to groups like radios or checkboxes, the ID in displayErrorMessages is the collection, but the ID in setElementErrorsFromFormState is the ID of the specific input and these don't match.

For example testing on umami search,

edit-type--error-message

but the element is

edit-type-article

referencing

edit-type-article--error-message

Definitely needs a lot of tests.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

neclimdul β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

From: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predef...

The HEAD SHA of the target branch of the merge request. The variable is empty in merge request pipelines. The SHA is present only in merged results pipelines.

Sounds good. Seems like this will work as long as we're using the correct type of merge pipeline. That also explains why we can get rid of the fetch so great news!

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

neclimdul β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

sorry for the noise. Forcing a 11.x branch to the issue fork to work around a bug in the core gitlab config.

The patch seems to be working great. Note about how we can make it better in the review.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

neclimdul β†’ changed the visibility of the branch 11.x to hidden.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

neclimdul β†’ changed the visibility of the branch 11.x to hidden.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

neclimdul β†’ changed the visibility of the branch 11.x to hidden.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

neclimdul β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

There's a lot to unpack in this merge request but wanted to point related to these changes the current spellcheck logic isn't great and probably not behaving as intended. The way it checks the remote branch actually checks against the issue fork which can relies on both the branch existing and actually matching the branch we're merging against. This only kinda works on recent forks but on _old_ forks it can be completely broken and fail entirely.

This probably needs its own issue unless this is close to being resolved.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

This seems like it is in a good place. The mitigation is pretty straight forward.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

This seriously affects the ability to administer sites without a documented work around so bumping the priority.

The code is pretty confusing and its not clear to me how it fixes the problem but i don't seen anything broken.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

I might be high-jacking this issue but it accurately describes the problem I've been tracking for a while now.

Quick tangent:

(queue workers starting to spit errors due to missing / expected properties.

This actually sounds like πŸ› Use item_id instead of qid for Queue item Needs review which actually happens when queue items are being processed. The lists being inconsistent should trigger claimItem to start returning a lot of false values which clients should interpret as a failed queue fetch and skip processing.

Theory of the problem

So I the problem comes down to how the garbage collection gets handled. GC is not atomic and can happen across multiple clients at the same time.

To set a scenario that might be able to reproduce this lets imagine a queue talking to an API and that API having some downtime. All those requests are failing. If the worker is smart enough to throw a RequeueException, those items would get immediately but if they crash the exception gets logged and the item is left to expire.

Later the items expire and GC kicks in. Lets imagine 2 workers trigger at close to the same time(they're hot looping claim item so this is likely) and get basically the same list of claimed items. They then loop through checking the leases. They're both going to see them same expired items and remove the claim and importantly _both_ push the item back onto the available items. This is a list not a set so we now have duplicate entries in the list which starts causing problems.

The best case is that 2 workers will be able to claim the duplicate item ids and work through them without a problem.

More likely later workers will either fail triggering the the same process or be unable to claim the item because it no longer exists.

Now the kicker that's been causing me headaches. Once a worker is able to process them item, claimItem will never be able to get the actual item because hget failures just get ignored meaning an expiring never gets set leaving the entry to be GC forever but also feeding back into the race condition.

Proposal

Making some things like the item list sets that can't have duplicates would probably be handy but I'm not sure how that would get implemented, if there are reasons for using a list over a set etc. So a couple of fixes can probably solve this.

Also, having two parallel lists of the available items is probably bound to lead to these sort of inconsistencies and this theory around race conditions might not be the only cause. So some sort of GC to keep it consistent is probably a good idea.

1. Implement a firewall so GC can't push items back into the available list if they no longer exist.
2. Have claim item clear inconsistent entries?
3. Implement some sort of cleanup like the exist patch that keeps the list consistent? The current approach looks a bit expensive for busy queues, should probably use exist instead of hget, and it might introduce its own race conditions so I'm on the fence for this.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Took a stab at automatically detecting if the program updating resource should be used. Best I can tell this is the right approach but the documentation is very confusing. Let me know if this help.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

So my understanding:

1. Documentation is bad. pushToMarketoUsingPOST requires a program name but doesn't document this requirement. It in fact documents the opposite programName (string, optional),
2. With formid, program name is bypassed. Maybe not a huge deal but a secondary bug.
3. Without formid, webform calls update lead. Without a program this fails because of #1.

The solution is then to provide a push and sync method and choose the correct one based on the program logic?

Marketo really made this so simple!

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

That's troubling but I'm not sure how to move forward with a fix without something I can recreate :(

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

This was fixed in the marketo library. Looks like the user found the fix.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

I'm having trouble finding any documentation on what the deprecation is and what the replacement is so can't move forward with a fix atm.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

We're in a good place. Thanks bot!

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

neclimdul β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

That's true, but we have a d10 release. We didn't but we do. Feel free to post some patches though, if there is some activity we can reconsider it.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

quick fix for style problems.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

I liked that idea and needed the idea for a project outside Drupal so I wrote this.

https://gitlab.com/neclimdul/phpunit-exceptions

Its a trait that works mostly the same but has some additional functionality and the assertions are accomplished a bit different.

Let me know if this would work for Drupal or if there are changes that would help.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

@alayham Yeah, the suggestion all along has been to avoid any localization and compare number/byte sizes directly.

https://git.drupalcode.org/project/drupal/-/merge_requests/670/diffs#not...

This was only in needs work because it doesn't have tests which is still true.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Skimming the discussion, I'm not sure which result was giving multiple seconds. Do you mean the _really_ small results that are in scientific notation with E-5?

I'm curious how this is able to generate hashes of files that are lazily generated. Some more digging to understand the patch I guess unless someone has a hint.

Marking NW and adding needs tests back because there are not tests in the patch. We can continue discussing performance impact outside that fact.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Maybe to limit the scope, keep this clean, and not build a circular dependency between this and the larger PHPUnit upgrade we can do s/There is no replacement/There is no replacement and test discovery will be handled differently in PHPUnit 10./ and then the CR has some block about following the PHPUnit 10 upgrade issue. At least if that's ok with committers.

We can then clarify everything when finalize the upgrade path.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

I'm not sure I was being clear since this is back to RTBC but my concerns aren't addressed.

Because of the way this is structured and the way the message is worded this looks like we're deprecating phpunit entirely.

Remaining self deprecation notices (2)

  1x: \Drupal\Tests\TestSuites\UnitTestSuite is deprecated in drupal:10.3.0 and is removed from drupal:11.0.0. There is no replacement. See https://www.drupal.org/node/3405829

  1x: Drupal\Tests\TestSuites\TestSuiteBase is deprecated in drupal:10.3.0 and is removed from drupal:11.0.0. There is no replacement. See https://www.drupal.org/node/3405829
    1x in UnitTestSuite::suite from Drupal\Tests\TestSuites

The problem is, there's no replacement for the class, but we should have a replacement in PHPUnit 10 for running PHPUnit tests.

I was asking that we figure out what that replacement is, then provide that explanation in the message and CR instead of just saying there's no replacement.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Turns around it was a little more complicated. I went ahead and took the opportunity to update the tests and make things a little more clear.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

"pretty sure the isset was the to preserve the behavior of the empty check as demonstrated by the failing tests." 🀷

Added it back because it was there to defensively handle a null value from the getPagerParameter method. Since isset is a language level feature there's no real cost to being defensive like this.

I'm already tired of the 500 remotes and all the commit juggling. I'm going to really miss the simple patch workflow. But ok, its in a MR now.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Wow! Not nearly as bad as I expected with all the dire warnings in the trait. Only the known views test is failing. Let me move that over to a kernel test and we'll at least have a green merge request.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

The script would have to be pretty complicated because the ways this get triggered are complicated. Code could be directly interacting with TestClass like core/tests/Drupal/Tests/Core/Test/AssertContentTraitTest.php which would only require understanding use statements and namespaces or something more complicated like a child class in core/tests/Drupal/Tests/Component/Utility/VariableTest.php which would require understanding LSB, static class instantiation, and the entire php inheritance of every class. Both are pretty complicated and the latter is _very_ difficult. That's why I suggested if we wanted a test, a static analysis tool that understands the structure of php code like phpstan would be the place to put it..

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Updated.

Turns out its been long enough a few things have changed. Specifically, current versions of PHP throw an exception instead of a warning which makes it a bit harder to see. Specifically, real exception gets gobbled up by a more generic exception and Drupal's exception logging in watchdog doesn't show previous exceptions.

Since the actual exception is hidden and its hard to validate the root cause without hacking the exception handler or using a debugger, I've left the original trace in place. The fix is the same and the trace captures the essence of the problem.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Its actually a PHP ArgumentCountError error because the require arguments for the constructor don't exist.

I've updated the IS with a specific examples of the error that this causes.

As far as preventing it that's a good question. In the short term, I'm not sure. But it looks like the plan in the parent issue is to support 2 versions and do upgrade testing like we did with previous PHPUnit upgrades. Once we get that PHPUnit 10 testing in place it would definitely start flagging weirdness like this. This issue was mostly part of getting obvious errors out of the way so the upgrade is a smaller easier to review task.

Its possible this could be caught in static analysis though and we _could_ reach out to mglaman about implementing a rule in phpstan-drupal to catch it. That might help contrib see the error before trying to upgrade to PHPUnit 10. It might also be overkill though because its technically code outside of Drupal. *shrug*

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Found another interesting issue exposed by the PHPUnit 10 updates.

πŸ› DependencySerializationTrait depends on removed __PHPUNIT_BOOTSTRAP global Active

The correct fix probably needs some architecture discussion because it would require changes to DependencySerializationTrait which affects almost everything.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

I assume this also triggers kernel test failures but I haven't gotten into testing Kernel tests yet.

Interestingly, the Unit test suite failure in ViewUIObjectTest::testSerialization exposes a test that isn't actually testing what it thinks it is. The test is trying to test that the object can safely serialize and unserialize. But because of this hack in DependencySerializationTrait, the unserialize component of the test "succeed" with a broken object because the hack allows the code to continue and silently unserialize without the container to populate the code.

This exposes that we can never really trust any tests of this code because they'll always behave differently in a test environment from a real Drupal site which is very bad.

This and the fact that we're embedding testing logic in production code which has its own code smell for performance and complexity reasons leads me to wonder if the entire hack should be removed in favor of addressing the problem somewhere else. Don't know if that's possible but it seems like it would likely be a better solution.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Created that followup meta to track replacing calls. πŸ“Œ [META] Convert use of InvocationMocker::withConsecutive() Active

The CR mentions the need to rethink tests and does a good job explaining the stop gap. I think that's enough. Its going to be a lot easier to provide specific examples of how a developer might think about replacing this logic after core replaces its uses. So providing that with a later deprecation of MockTrait::consecutiveCalls makes more sense IMHO. We can provide some links between the CR's at that point to make them more findable as well.

All that to say, agree this is RTBC.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

So yeah, I was hopeful about the event and extensions but after tracing up and down there's not much to be done. The classes need to go away, just not sure what the path forward for supporting our various testing logic. Mostly discovering tests in submodules and contrib.

Added the original that added discovery logic to summary.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Spent a bunch of time digging into the discovery process related to the test suites and that sent me down a lot of rabbit holes. The test suite part I did not enjoy but it ended with me having a mostly working unit test suite so that's great! Of course mondrake was ahead of me on basically all of those changes.

Tried to capture all of my findings and the various work happening in an updated IS.

Will capture some issues for some of the things I found that I think can be addressed in the short term.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

neclimdul β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Quick patch to confirm the return types don't cause problems in PHPUnit 9.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

"But perhaps we just shouldn't support that, as Sebastian Bergmann is trying to discourage use of this I think we should too - I feel this should only be for converting existing methods if at all possible, and we should try to find alternative approaches for new tests."

I agree with this sentiment. This sort of approach is pretty brittle and I'd discourage it too. I expect there are various approaches like mocking less, using prophecy to tie the returns to specific arguments, or using data providers to deal with variations on duplicate code paths.

I also wouldn't hold this up refactors all of these though and all the individual reviews associated with that. Each one likely has their own unique problem and requires complicated understanding of the related system. This is a elegant stopgap.

So my only questions are:

  1. Do we need to address this: "Do we need a CR in this case, if we are not suggesting to use the method?"
  2. Should we create follow ups for removal/updates?
πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Yeah, hidden was one of the approaches investigated earlier. The quote summarizes things pretty well.

"the li is hidden, but its content is not"

Without this weird presentation behavior, there is a fork in the road.

  1. Keep the content by keeping the li and making it more accessible.
  2. Hide the content with the li.

"... if you want it ignored"

Since both approaches address the issue from a test perspective, I went with the USWDS approach assuming it provided a better experience in their testing. I think the idea is the ellipsis provides information for why the numbering skips. So instead of hiding it completely, we take the first approach, keep the li and provide more information.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Looks like this code is behaving correctly but maybe media is being naughty?

Basically, media has media_library_views_post_render which calls MediaLibraryState::fromRequest($view->getRequest()).

You might think this is generating a media state from the request but it _also_ mangles the request on the view.

    $query = $request->query;

    // ...

    // Remove ajax_page_state as it is irrelevant.
    // @todo: Review other parameters passed
    // See https://www.drupal.org/project/drupal/issues/3396650
    if ($query->has('ajax_page_state')) {
      $query->remove('ajax_page_state');
    }

Because the views request is the _actual_ request and not the cloned version, this is mangling was able to remove the actual page state. But when this patch adds the state back it breaks this little hack which apparently doesn't have a test. Or more accurately, it kinda had a test we are now removing in this patch. However it didn't test that it's hack was actually changing view's ajax behavior so it didn't see this change as breaking.

So... maybe this is blocked on πŸ› MediaLibraryState passes all query parameters around, not just the ones it needs Active ? I'm not sure what the next steps are because I'm not really familiar with what media is trying to accomplish and why it is fighting with asset management like this.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Well hello old me. Found this again testing PHPUnit 10 changes. Don't think its technically currently a blocker but gets in the way of testing so would be good to get out of the way since its such an easy fix.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

I'm aware symphony does all the writing in drupal. We don't touch the encoder though and the test asserts the parsed yaml matches the value of the constant, not that the constant format is in the encoded yaml.

I'll do some manual testing in a few days to make sure I'm not missing something though.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Looking through the code, I expect this is probably only reading constants not writing them unless there's some magic I missed so the title/scope should probably be updated to avoid confusion.

That makes this only useful to developers writing config directly like comments that also cannot be sync'd but that was probably always the only reasonable scope. I think that's still worthwhile for maintainability of core and contrib so +1

Our CR should probably be clear about the side effects/impact of this though. If you write a constant in your install config, then at some point in the future change that constant's value (like bumping password strengths or something) it won't change installed config on sites. You'll need to provide a method for user's to get that updated value. This is also going to be hard to test for because your test site in CI will get the new value during install masking the impact.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX
  1. +++ b/core/misc/cspell/dictionary.txt
    @@ -313,6 +313,7 @@ druplicon
    +eacute
    

    This isn't in the patch? Why is it getting added?

  2. +++ b/core/modules/layout_builder/src/EventSubscriber/SectionComponentVisibility.php
    @@ -0,0 +1,88 @@
    +      // If conditions do not resolve, do not process other subscribers.
    +      $event->stopPropagation();
    

    This relies _very_ heavily on the render order. Is that a problem? What happens when modules mess with the render order? Can we make this more resilient or move it down a layer?

- UI is ugly? Don't know if this is something with my admin theme but all the components are on the off canvas overlay but they're hard to understand and interact with.
- Adding visibility closes the entire off canvas area instead of returning to visibility control. This makes it quite awkward to make multiple changes or even confirm your changes where done correctly.
- Sometimes we say control visibility, sometimes we say configure visibility.

@vasike earlier you mentioned questions not getting answered. Are those still relevant? Can we recapture any outstanding issues in the IS?

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Thanks catch! I was definitely thinking about moving towards that so glad you agree.

As far as the 10.1 patch, you should be able to use the earlier version. it doesn't have the docs but behaves the same.:

https://git.drupalcode.org/project/drupal/-/commit/827b13b4bd8beaf77861b...

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

So I started doing that and then I'm like "Well, this isn't really different then '_drupal_ajax' so we should probably use a similar constant so we can track dependencies in the same way.

Unfortunately, this was like pulling up the proverbial rock and all the bugs rushed out. The reason it didn't "affect" core was it seems like there are a number of tests and workarounds placed directly in various core systems. Specifically Pager and medial_library filter this and only this parameter out of some URL building.

I've gone ahead removed the work arounds, cleaned up the documentation related to these query filters, and consolidated the filtering list to hopefully make systems that need similar workarounds in the future easier to implement.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

We originally tried to do this in directory definitions but eventually had to give up because it was so dynamic and varied with the structures Drupal can exist with. PHPUnit is also pretty limited in how you can define these. I don't think that requirement hasn't really changed so I don't think we can deprecate without a replacement.

I don't think this is a problem though, if we're clever, we can probably move the logic into the events available in PHPUnit 9+ with probably opens up a lot better functionality and growth. We should even be able to do a deprecation in Drupal 10 without converting fully to PHPUnit 10. Just triggered if the static method is called directory and then pointing users to update their local configuration along the timelines of the PHPUnit 10 to what ever the event system looks like.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

I'm not sure I agree with the understanding of the issue.

1. run-tests is _one_ way to run tests against Drupal. The existence and documentation of the use of core/phpunit.xml.dist is the other. Run-tests is so clunky, awkward, and slow I haven't used it in 10 years or more at this point? It works for testbot but I can't figure out how anyone would use it regularly.
2. moving away from exposing our tests to phpunit blocks deeper integration into the phpunit ecosystem which I feel like is _not_ where we want to be heading. Not running tests in your IDE natively, not being able to look at paratest, etc.

So we don't need to deprecated it, we need to replace it.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Don't see any discussion on it but our test suite registration is no longer supported in PHPUnit 10.

https://github.com/sebastianbergmann/phpunit/commit/36354a9cdc69ecabd20a...

Might be a really cool solution in the current patch I'm missing though.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

@laurii Took a look at the a11yTestDefault test again today and I'm really unsure what to do. It "Sounds" like the correct location because it would ensure we're testing this as standards change. I have lots of questions though because its just a list of URLs which doesn't provide a lot to work with and the core documentation doesn't seem to cover it.

  • How is the site is setup? What's available to test against?
  • How to affect things so that there is a URL that has enough pages to trigger the pager overflow? My guess is the site is probably empty or minimally populated so most urls with a pager won't have overflow in the pager.
  • Can you somehow assert the element is on the page so the accessibility test is doing something? If we're mocking a bunch of content and then just have a url in the test file, it would be very easy to regress to the point of not testing this anymore without knowing it by removing(optimizing) content.
πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Random test failure πŸ› [random test failure] TimestampFormatterWithTimeDiffTest::testTimestampFormatterWithTimeDiff Active in Javascript tests. Nightwatch looks like chrome crashing?

Seems that change fixed the problem.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Should probably skip and fix all the TimeDiffViewsTests doing these sort of assertions. They're all failing like this it seems.

TimestampFormatterWithTimeDiffViewsTest::testTimestampFormatterWithTimeDiff is failing too: #3401720

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

neclimdul β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Investigating something yesterday I think I figured this out. It might require referencing Wim's D8 render pipeline flow chart and understanding that `ajax_page_state` actually happens in a AjaxResponseSubscriber though. It boils down to, since we're _entirely_ removing the query from the request in the controller, later when it gets to the response subscriber its also gone. The subscriber then thinks it never existed and doesn't recompute the libraries to return correctly. This then can cause problems for interactions with the page since the libraries that previously existed on the page have been removed from Drupal's list its tracking.

There's _probably_ an argument to be made that AjaxResponseSubscriber should capture the query during the response event and hold it to be more resilient to things happening during the rest of the request handling. I'm not sure if that's the correct way or if there's an appetite to change it and its definitely a big change of scope from this issue so going to try avoid addressing it here.

That said, since this code is already specifically trying to handle this sort of request hackery, we can do a little more to work around this limitation. The merge request has been updated to hopefully handle handle things. If it works it could probably use some documentation on why it needs to work the way it does and maybe a link to an additional follow up.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

"Facets 3 will not support AJAX when using Facet blocks."

This seems like kinda a broken model. Facets usually lives in a separate location on the page from exposed filters like the search text. So merging them like this seems like it breaks the most common use case for using facets.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Fix deriver base class name.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Based on the rest of the comments and the lack of tests don't think this is actually ready. There aren't even tests on the last couple patches.

re: #32 what the heck is that test. Its not in the system module, its in the wrong location based on the namespace of the class, the file name doesn't align with the tested class? Something weird going on there but it does look like that's where existing tests are.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

The new scope you defined was but I don't feel like the original intent was. There are lots of inclusivity reasons and just general choice reasons someone would choose a server outside the to provided options.

I actually appreciate the effort a lot and the motion on this, just think we can do better.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Yeah, doesn't really makes sense but they did fail. Some of the failure messages don't make any sense though so going to do some manual testing to get to the bottom of things.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Doesn't sound like we really addressed this in the decentralized way mastadon actually works.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

I'm not sure I understand what got fixed. My Mastodon link looks the same.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Sounds like a better candidate then me writing a browser test. I don't really have any much experience with the nightwatch tests and none with this test which seems to maybe have some magic I don't understand.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

This is _very_ broken and seems to be tied up in one way or another in every commit on the 3.x branch. The BEF changes touch it, there #3210353: Issue with browser Back button and Checkbox widget β†’ hacks and complicates things, and then #3309612: Link widget styling β†’ tries to "Make it a a link" again by styling the span blue? Not only does that hit themers bad its the amazingly not accessible.

I don't know how to fix this but there seems to be a fundamental project with how facets is thinking about link widgets now.

This seems like it should be a blocker for 3.x so marking it critical.

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

neclimdul β†’ made their first commit to this issue’s fork.

Production build https://api.contrib.social 0.61.6-2-g546bc20