πŸ‡¬πŸ‡§United Kingdom @alexpott

πŸ‡ͺπŸ‡ΊπŸŒ
Account created on 27 June 2007, over 16 years ago
#

Merge Requests

More

Recent comments

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Committed and pushed 02c07301fc to 11.x and 86594c44d6 to 10.3.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ created an issue.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Committed and pushed 213906e286 to 11.x and 522404706e to 10.3.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Committed and pushed 1e4c8cd1a70 to 11.x and 0963e02510f to 10.2.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Added some comments to the MR that I think need addressing.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Committed and pushed 21216ed8d00 to 11.x and 76055eeffe1 to 10.2.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Removed the boot() changes. We need to handle container the container better in \Drupal\Core\Recipe\RecipeRunner::processRecipe too... - it's a hard problem. The solution is probably to pass the kernel object around but I think we should address all of this in one issue and not do anything piecemeal.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Committed and pushed 07a3ac8477 to 11.x and d552da855a to 10.3.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Committed and pushed 44c345dd9b to 11.x and 2172d1009c to 10.3.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Discussed with @catch.

There's one small interesting behaviour change. The new way of immediately adding messages via ajax means that you're more likely to get duplicate messages on the screen because you don't benefit from automatic deduping by the messaging system. If this turns out to be a big issue we can look to do the deduping in the messages AJAX.

Also it is very odd that the markup is different - but themes already have to cope with this so it is not new.

Given the benefits going to commit to 11.x and 10.3.x.

We considered a change record for the above behaviour changes but decided that it would be noise as there is nothing obviously actionable or an easy way to see if you will be affected.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Backported to 10.2.x as a test only change.

Committed and pushed eed7ea3a50 to 11.x and 0d4a5883d4 to 10.3.x and 4d9ec8d74a to 10.2.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

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

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Tests on the postgres mrs re green on all db drivers. Yay

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Addressed @andypost's review

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ changed the visibility of the branch 3405976-10.1.x-shutdown-functions to active.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ changed the visibility of the branch 3405976-minimal-approach-shutdown-functions to active.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Created two MRs one for 11.x and 10.3.x and one for 10.2.x that fix the postgres test. That particular test is pretty icky - I did nearly remove it at one point :(

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ changed the visibility of the branch 3405976-minimal-approach-shutdown-functions to hidden.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ changed the visibility of the branch 3405976-10.2.x-shutdown-functions to hidden.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ changed the visibility of the branch 3405976-10.1.x-shutdown-functions to hidden.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

@smustgrave this is not a highlight yet. We need to fix the underlying Drupal tests to be W3C compatible.

Moved deprecation to WebDriverTestBase - this will allow us to differentiate between core and custom/contrib tests in the future which might make things easier.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

@andypost we've not changed the protocol in this issue. W3C is set to false by default here. This is about getting the dependency change done. Chromedriver doesn't care whether you use W3C commands whatever W3C is set to. So in order to prove we're using W3C we have to do πŸ“Œ Use selenium/standalone-chrome instead of our chromedriver image Needs work or something similar.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Thanks @mondrake for the review - I've responded on the MR to your questions / points.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

@vasike I do not understand why we want to change this. We can add the @see's so people are not confused in the future and be done.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

We have MRs for 11.x, 10.2.x and 10.1.x

I think this is as good as we can make it.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ changed the visibility of the branch 3405976-minimal-approach to hidden.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Discussed the two approaches with @catch and @longwave and we all preferred the shutdown function approach because:

  • It is more self-contained
  • fixes all the situations where this can occur without having to mandate that kernel terminate is called
  • Does not require changes to custom or contrib code.

Updated issue summary accordingly.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

After sleeping on this I realised that we can fix this in a more reliable way - we can trigger a shutdown function if a transaction is created. This way we don't need to worry about kernel termination. Re-opened that MR and here's a 10.2.x patch for that approach.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ changed the visibility of the branch 3405976-minimal-approach-shutdown-functions to active.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Unfortunately we can't test the locks being release either as that is caused by the transactions not being committed- so everything is caused by PHP's object destruction not occurring as we expect - and we have no way to cause this other than with xdebug in develop mode.

So I don't think we can add a truly meaningful test here. What we can test is that calling terminate or shutdown causes database transactions to be committed.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

@solideogloria we can't test the xdebug mode issue because we don't have xdebug running on gitlabci - but what we can do is test that locks are released after calling drupal_rebuild().

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Updated issue summary and title inline with current approach on the MR.

Note we will need to fix https://github.com/Lullabot/php-webdriver/pull/9 before we can get a green run.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

The problem with your suggestion is that it still allows for clashes consider a site the following entity type names:
two
two_tag_alpha

If there's a query with the tag alpha there will be a clash. Yes this is very contrived but someone somewhere will have a set up like this. This is why there are no hooks with SOMETHING_hardcode_SOMETHING. It's really hard to come up with something that will not clash. Also this is why we should move away from hooks and to something like hux β†’ .

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ changed the visibility of the branch 3405976-minimal-approach-shutdown-functions to hidden.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ changed the visibility of the branch 3405976-minimal-approach-shutdown-functions to hidden.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Here's a patch that should hopefully filx @solideogloria's problem and call terminate consistently on kernel's in core. It's also going to solve locks not being released by drush cache-rebuild - so that will be improved too... going to look for an existing issue about drupal_rebuild not releasing locks because my guess it that there must be one!

Going to drop the shutdown approach because it adds complexity and if you are doing database transaction in shutdown functions then I think that is quite a special case.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Thanks @solideogloria - excellent information - I can now reproduce the bug you're seeing and can fix it. Patch incoming!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Updating the issue summary to outline the more recent solution.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I've pushed a MR that:

  • Does what @pwolanin asks for in #26 - rolls the functionality into the jwt_auth_issuer module
  • Adds test coverage
  • Adds an update path for the new config object
πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Re the last part of #60 it's not quite the same as sql's alter tags though... because you are adding the tag on to both entity_query and entity_query_ENTITY_TYPE_ID

Also if a tag and entity type ID clash your site will have problems and that means this needs work.

I think it needs to be something like

      $hooks = ['entity_query', 'entity_query_' . $this->getEntityTypeId()];
      foreach ($this->alterTags as $tag => $value) {
        $hooks[] = 'entity_query_tag_' . $tag;
        $hooks[] = 'entity_query_tag_' . $this->getEntityTypeId() . '_' . $tag;
      }

to avoid clashes.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I've moved the commits even later during execution - moved them to shutdown functions. This has some interesting effects on kernel tests which mean we need to commit transactions when we close connections - which makes sense to me. That in turn makes \Drupal\KernelTests\Core\Database\DriverSpecificTransactionTestBase::testTransactionManagerFailureOnPendingStackItems() pretty meaningless in its current form.

@solideogloria does this change in approach fix things for you? New 10.2.x patch attached.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Sorry I'm hung up on the custom thing like @larowlan was. I've added a comment on the MR with a suggestion to change this from detecting custom - because that has way too many ways of being wrong in both directions to checking whether the template comes from the active theme.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I'm not sure about this. What happens to IDE completion with this? I think IDE completion of plugin attributes is more than just a nice to have.

I think we could detect missing base variables in a different way - like we could change the validator here to ensure that the plugin constructor contains a superset of the arguments of \Drupal\Component\Plugin\Attribute\Plugin::__construct() and preserve the IDE capabilities.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Looking at how SQL queries are altered - it only triggers the alter hook if the query is tagged. See \Drupal\Core\Database\Query\Select::preExecute(). I'm wondering if we should do the same. Ie. change the code to

  protected function alter(): Query {
    if (isset($this->alterTags)) {
      $hooks = ['entity_query', 'entity_query_' . $this->getEntityTypeId()];
      foreach ($this->alterTags as $tag => $value) {
        $hooks[] = 'entity_query_' . $tag;
        $hooks[] = 'entity_query_' . $this->getEntityTypeId() . '_' . $tag;
      }
      \Drupal::moduleHandler()->alter($hooks, $this);
    }
    return $this;
  }

Hmmm are all entity queries tagged... ah interesting, is that some tags that you'd expect to be there like ENTITY_TYPE_ID_access are only added in \Drupal\Core\Entity\Query\Sql\Query::prepare() which is called after this. Unfortunately in the current design it does not seem possible for a hook implementation to know if \Drupal\Core\Entity\Query\QueryBase::$accessCheck is TRUE or not.

Also I'm wondering about the impact on the existing entity_query and entity_query_ENTITY_TYPE_ID tags. They feel somewhat superfluous and duplicates.

Another thought is in what order should the hooks be called. Obviously hook_entity_query_alter() first... but then I think it should be hook_entity_query_TAG_alter() then hook_entity_query_ENTITY_TYPE_alter() and then hook_entity_query_ENTITY_TYPE_TAG_alter()

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

You still have 250000 blocks on your site which cause problems elsewhere. This feels like a temporary fix. The real fix feels like we should replace the derived content block plugins with configured content block plugins. See πŸ› block_content block derivatives do not scale to thousands of block_content entities Needs work for an extensive discussion about this. This issue is a sticking plaster... and to quote an old friend... what we're doing here is sticking a plaster on a very wet leg.

Committed baf0e40 and pushed to 11.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

This bug was introduced by #2981887: Add a publishing status to taxonomy terms β†’ . the fix seems reasonable. I do wonder if status should be accounted for in \Drupal\taxonomy\TermStorage::loadTree() as that is adding the tag 'taxonomy_term_access' to it's query but afaik this is no different to nodes.

Committed and pushed f660bb4c1e to 11.x and 9397852246 to 10.2.x. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

The current approach would be a BC break since before a value of FALSE would not add ajax but now it would. Also a value of TRUE would be not add the ajax default so custom and contrib would be broken.

I don't think it is worth changing the behaviour here. I think it is worth added @see's to where #ajax is being set to TRUE so we don't wonder the whys and wherefores in the future.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Yeah - now that config install is part of core this can be closed. Just have the entire config directory in your profile and be done.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I still think we have problems with the PHPUnit tail wagging the Drupal dog and removing PHPUnit from the dependency chain would be great. There's always a tension about having dependencies on tools.

That said I think closing this task is okay as no one is actively working on this.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Looks great and from the πŸ“Œ Use selenium/standalone-chrome instead of our chromedriver image Needs work issue we know it fixes the problem.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

@solideogloria does the bug only happen when you clear the caches with Drush? Do you all get the error when you clear via the UI. Just wondering if it is something special about using Drush?

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

@solideogloria yeah if you can find out what's causing this that'd be awesome. If you could share your composer.json and core.extension configuration with me that might help. Might some contrib that's causing the problem. Or maybe (even better) there's some way you could share your build with me. Are you on slack β†’ ?

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Here's another patch for 10.2.x that has the latest changes that changes thing to do a commit on all open db connections. @solideogloria it's really interesting that disabling xdebug seems to fix your problem but not changing the mode. That's the first reported instance of this. Can you confirm that you only get the errors with xdebug enbled? Also can you try the latest patch attached here as you did mention that you have multiple connections.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

@solideogloria I've got a couple of questions about #93. Can you confirm that the bug goes away if the php.ini setting xdebug.mode does not contain develop. I.e. it is something like xdebug.mode=debug

Also can you try with the patch attached as this might reveal the error that's causing the error... it makes a small change to _drupal_get_error_level() so errors in error handling don't get in the way.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I'm pretty sure this is the wrong fix.

We hit this error in πŸ“Œ Use selenium/standalone-chrome instead of our chromedriver image Needs work . It occurs from the second call to checkMutation in this code.

  function checkMutationAndProcess(node) {
    if (checkMutation(node)) {
      processReplacement(node);
    }
    // Checks if parent node of target node has not been processed, which can
    // occur if the script node was first observed with empty content and then
    // the child text node was added in full later.
    // @see `@ingroup large_chunk` for more information.
    else if (checkMutation(node.parentNode)) {
      processReplacement(node.parentNode);
    }
  }

There's no way that node can be a null but there is a way for node.parentNode to be NULL. In that issue we changed the code to:

    // occur if the script node was first observed with empty content and then
    // the child text node was added in full later.
    // @see `@ingroup large_chunk` for more information.
    // If an element is added and then immediately (faster than the next
    // setImmediate is triggered) removed to a watched element of a
    // MutationObserver, the observer will notice and add a mutation for both
    // the addedNode and the removedNode - but the referenced element will not
    // have a parent node.
    else if (node.parentNode !== null && checkMutation(node.parentNode)) {
      processReplacement(node.parentNode);
    }
  }

Yes the code changed here will result in fixing the problem but I think the comment and code flow are better with the changes in πŸ“Œ Use selenium/standalone-chrome instead of our chromedriver image Needs work .

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

After discussions with @catch we decided to skip core performance tests on Postgres and SQLite because this is not really fixing the extra queries on postgres because they are due to the driver. So I reverted the postgres specific fix and added a skip in PerformanceTestBase.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

The latest commit on the MR is a nice colour of green for postgres... https://git.drupalcode.org/project/drupal/-/pipelines/96195

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Fixing the issue title.

Re #9 I'm not that comments are really possible. These number really represent a stake in the ground - this is what Drupal currently does. If we introduce something that improves them then we want this test fail and the issue that makes the improvement should adjust the numbers. If we have an issue that degrades them then we need to carefully consider why and if it worth it or if there is a better way.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

The postgres fails are nothing to do with this issue - they're happening in HEAD - see #3420401-10: StandardPerformanceTest::testLoginBlock Failed asserting that 50 is identical to 49. β†’ ...

@solideogloria / re #118 I'm pretty sure that was a patch applying issue - got exactly the same fail on Thunder (see https://github.com/thunder/thunder-distribution/actions/runs/7920011867/...). The patch in #122 fixes that.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ changed the visibility of the branch 3405976-with-3421164 to hidden.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Yep as you can see from https://git.drupalcode.org/project/drupal/-/jobs/828846 this still does not fix postgres.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

This very nearly fixes a critical regression in Postgres testing - head is failing due to this. See https://git.drupalcode.org/project/drupal/-/pipelines/96003

But unfortunately there's one more thing to fix in Postgres... will push a commit to MR once the current MR has finished testing on Postgres.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

@solideogloria can you supply a full stack trace \Drupal\Core\DrupalKernel::initializeServiceProviders() does not call terminate. Plus can you provide more information about what you're doing when things do not work. As far as I can see you are doing a drush cache rebuild... this works fine for me. Are you sure the patch as applied correctly? We're not changing any line near 1360... I'm attaching a patch for 10.2.x that allows me to install Drupal and rebuild the cache using Drush as the MR does not apply to 10.2.x. Please try this instead.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

We have fails in Drupal\Tests\standard\FunctionalJavascript\StandardPerformanceTest to work why this code is causing extra queries there... but not on SQLite or MySQL...

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Thanks for the review @catch - I've made those changes.

@solideogloria can you re-test with the latest MR. If it doesn't work can you give as much information as possible so we can work out why. Thanks!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I've just tidied up the PHPStan and PHPCS jobs... this looks great. Nice work @pbonnefoi

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

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

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Thanks for the review @mondrake. I left replies to your comments on the MR.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Okay so Thunder's tests pass with the minimal mr with all the installer kernel termination fixes... https://github.com/thunder/thunder-distribution/pull/773

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

TIL the kernel is not terminated in the interactive installer during installation. Wowzers.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I've opened an MR that uses service destruction and the kernel to work around this bug. It's a more minimal approach to resolving the issue in 10.2 and 10.1 that results in me being to run the \Drupal\Tests\mysql\Kernel\mysql\TransactionTest successfully locally with xdebug 3.3.0 in develop mode. If I run against HEAD it is broken horribly.

Yes it would be great to fix in a more explicit way but Drupal's transaction committing has been implicit for a very long time so I think moving away from the properly is going to take time and a lot of work.

@mondrake what do think about doing something like https://git.drupalcode.org/project/drupal/-/merge_requests/6614 as stop gap and then trying to fix this probably in another issue.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ created an issue.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Re #60 the file cache usage in \Drupal\Core\Config\FileStorage is only in memory storage and not APCu cache. So if we read the schema twice in a request then we'd benefit - but not after a cache clear.

I think this is okay as we don't actually read schema during a cache clear or regular use of your site. But I also think we should open an issue to add schema to the APCu cache.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

@smustgrave it's definitely not random.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I've resolved @longwave's feedback but I have some concerns that we have failures on a more recent chromedriver than the one we're currently using. Moving to https://hub.docker.com/r/justafish/chromedriver/tags to prove this.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

The lullabot deps already ship with replace sections so #39 is moot. Nice.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Crediting @justafish as a tonne of the effort here is theirs.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

The one thing I'm wondering here is should we use a composer replace here to indicate that the lullabot dependencies replace the other ones... not sure.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Created the issue to use selenium/standalone-chrome - see πŸ“Œ Use selenium/standalone-chrome instead of our chromedriver image Needs work ... going to close the other MRs here and move the bulk of that work over then.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ created an issue.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Discussed with @justafish - we agreed to split this issue into 2.

This issue will move us to using a W3C driver for JS testing and then a follow-up issue will look at swapping out our chromedriver image for a selenium standalone image.

Will create an MR with the minimum change for us to use the W3C driver from lullabot.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ
πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Crediting @justafish for all the work to get this issue progressing. Nice one.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ changed the visibility of the branch 9.3.x to hidden.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

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

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ changed the visibility of the branch 3240792-use-php-webdriverwebdriver- to hidden.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

@smustgrave not really - Yaml reading is such an integral part of core that this code is called 1000s of time per test run. It's tested - the thing to check is the deprecations and does everything make sense.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Creditting the authors of the previous MRs.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I agree with #28 we should to do the minimum here and maintain maximum compatibility for tests. I agree that postponing deprecations to a follow-up makes a lot of sense.

Given the how some of the typehinting has changed in behat/mink 1.11.0 it's not possible to wrap everything in a deprecation but we should do a best effort.

I've opened πŸ“Œ Update Behat from 1.10 to 1.11 Active to do the deprecations and I will create a new MR based on the all the previous work on this issue and close the two previous MR that it is built from. This will allow core-dev-pinned to get to the same version as core-dev and allow the W3C issue(s) to proceed without having to do to this upgrade too.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ
πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

alexpott β†’ created an issue. See original summary β†’ .

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

It would be great if this functionality was rolled into the JWT module. I've got a client site using https://www.drupal.org/project/getjwtonlogin β†’ for this functionality but it definitely feels as though it belongs in the JWT module. I'm going to add tests and try to implement #26.

I will also add test coverage.

There's one more thing that this issue opens up. There are questions about session. If you use user.login.http a session will be generated when you log in. However if you only use the bearer token you've been given for authentication then the session will not be maintained as sessions in Drupal work via cookies. A cookie will be returned on the login request but you'll need to extract it and use it in your app which feels like a duplication of the work being done for the bearer token.

Therefore I think we should do two things:
1. Make the creation of session configurable
2. Make it possible to use the bearer token for session.

Given that these are existing issues I propose that we address this on follow-up. That said, I do have some concerns that making the creation of session configurable will necessitate significant changes to the approach of decorating the user.login.http using the response event.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Addressed @larowlan's feedback - thanks for the review!

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Thanks @mondrake for fixing it to have an unlimited timeout - yeah 60 seconds was just not going to cut it :D

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ
πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

@longwave yep - opened πŸ› Remove new test timeout Needs review to address.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

I've made this major because this will lead to contrib and custom tests failing.

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ
Production build https://api.contrib.social 0.61.6-2-g546bc20