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

KVUO
Account created on 11 January 2006, about 18 years ago
  • Staff Drupal Engineer at AcquiaΒ  …
#

Merge Requests

Recent comments

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

This is by design. The machine name and label are auto-generated because requiring users to create them is an additional unneeded step. Is there a reason why they needed to be edited specifically?

Note -- if there is a need to change them, it can be done manually with the config entity. Since they really shouldn't be changed by end users, I'd suggest going this route instead of changing the UI.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

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

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

Will need to retest after moving to Gitlabci -- I think the tests are still failing here.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

At first I was confused why you'd want to do this, but yes you're right -- in case somehow you ended up with this box unchecked, you should be able to check it even if other containers are there.

Fixed.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

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

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

It appears the new test infrastructure is failing tests now.. it'd be nice if those were fixed in this PR.
https://git.drupalcode.org/issue/google_tag-3420026/-/pipelines/90392

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

Fair enough use case indeed -- however tests are failing so moving to needs work until that issue can be uncovered.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

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

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

I agree about the idea of a tempstore before firing, if thats even possible.. But there is likely some work needed to get this issue totally fixed per Berdir's comment.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

On first glance this looks great! Waiting to see if I can get the new test suite to pass, I committed some changes a week ago that fixed tests for Drupal 10.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

Incremental progress is still progress, so gonna just commit this. If users need more functionality they should open a new issue and reference this one.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

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

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

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

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

Add default settings to gtag.js. Its not configurable from Drupal at the moment.

One thing that needs validation is the 'wait_for_update` variable. In this issue it suggested 1500ms. Google suggests 500ms. It'd be good to see if a third party consent manger works properly with this delay.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

We're currently working with google to implement this in the Google Tag module. (2.x). Note, if this wasn't implemented, analytics would be significantly curtailed on March 6th: https://support.google.com/google-ads/answer/14505993?sjid=1828950924013...

The default configuration will add the 'denied' option by default to all tags. Supposedly the 'wait_for_update' parameter will allow other drupal modules (or third party CMPs) via Javascript to change the setting to accepted when a user consents to tracking.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

japerry β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

Yes, there is good cause to be worried. While acquia search would support Search API Solr 4.3.x (we're just a connector, and doesn't appear that the class APIs have changed), it would require customers to download a new configset.

It also means that we should update our 'default' configsets as well. I've let product know about it. Likely the resolution will be a note in our knowledge base and an additional configset option. Since customers are in control of their search configuration, it will be on them to migrate when its appropriate.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

I spent some time on this today and there are a few things to point out:
1) The original 3.5 code enabled ALL drush plugins by default. Only 3 were supposed to be 'enable_by_default':

drush_purge_queue_add
drush_purge_queue_work
drush_purge_invalidate

I don't -think- enabling the other plugins by default would cause the errors in the database query, but its untested, so who knows that could be it.

2) The 'enable_by_default' is sorta messy with purge. It means plugins are enabled by the mere existence of the module. This means that for those using the above drush commands (and associated plugins), they will DISAPPEAR if you disable purge_drush. You must manually re-enable the modules via the other drush commands (p:processor-add drush_purge_queue_work) or via purge ui.

3) The purge_drush module, with the above PR (purge_drush_fix) only serves to enable those three plugins by default.

4) The plugin cache gets automatically rebuilt when you run updb and have purge_drush enabled or if you manually disable purge_drush and run manually run updb. The classes are pointing to the purge core src directory. The command classes in purge_drush effectively do nothing, with the exception of those who may have extended them in their own custom commands and added annotations.

I'll give this a few weeks (since its christmas 2023) and then commit it if I don't see any responses.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO
πŸ‡ΊπŸ‡ΈUnited States japerry KVUO
πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

Please do not do drive-by patches. We're rolling out something in the next quarter for all acquia projects using gitlab ci.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

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

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

We can simply replace the NULL with '' as default in variable_get, as thats whats causing the null error.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

Closing as this isn't really an issue in the module per se.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

This might be a core issue actually..

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

Yup that is correct. (for the most part... in advanced uses default may be a multisite install)

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

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

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

Going to mark this as fixed, but there are some real issues with how memcache is wired in the tests. I can reproduce all the errors that are found in gitlab ci, so we should assume that tests upstream are broken. Getting this in will help us debug and fix the tests.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

Anywhere we're creating a new instance of the plugin, ($this->pluginManager->createInstance) we're likely needing to wrap them:

  • IteratingServiceBaseTrait.php
  • InvalidationsService.php
  • PurgersService.php --> note the Akami patch already does this
  • QueueService.php
  • PluginTestBase.php
πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

I think this issue is tangentially related: https://www.drupal.org/i/3391383 β†’ -- we need to handle disappearing plugins better. Maybe some of the work in the patch there can be used to make handling the missing drush plugins uninstall correctly?

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

The issue here is the plugins that were moved are still cached which is causing the error. The next version needs to bring back those plugins and then figure out a way to show them as deprecated so sites start using the new plugin instead.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

Huh interesting.... the reason why its not removed is that when you delete a submodule it will cause a WSOD without manual db intervention. Therefore for the entire 3.x branch, the module needs to remain, even if we have it hidden.

The reason why we didn't automatically disable it is because sites could have been relying on the old namespaces, and disabling the module will kill their code. But in theory it should have worked along side the new modules even if enabled.

For API purposes, it should be using the libraries within purge itself. So its odd that its failing. I'll prioritize some time in the next weeks to look at it.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

Yup, that is fine. That there is a team at Acquia that is now working on the module/drupal side of things, including Purge (and Acquia Purge)

If there are support requests that come in for D7 we might be able to take a look, but my experience is also D8+.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

We'll look at adding it as default later on. so for now, fixed!

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

Thanks for the background. Pushed the typo up to head. Fixed!

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

Sorry I'm unable to reproduce. A few things to note:
1) If you're using media: Acquia Dam, you'll be using the Entity Browser to get assets. If you setup the entity browser and select Acquia Dam Video (not asset), you should be able to choose videos and they will embed on the content.

2) If you're using the newer Acquia Dam module, you can use the media library. you can follow these instructions: https://community.acquia.com/acquiadam/s/article/How-do-I-configure-the-...

You can use both modules at the same time, but you'll have to setup the process (entity browser vs media library or both!) in order to view the video assets. I wouldn't add items to the Dam Asset type.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

We've ran into this bug from a slightly different angle with Acquia DAM. We have implemented our own Metadata Sync controller which will attempt to resave the media entity. However the following code in the media entity ensures that if you change the metadata on the external source, it'll never update Drupal because the actual source asset id hasn't changed.

// Only save value in the entity if the field is empty or if the
// source field changed.
if ($translation->hasField($entity_field_name) && ($translation->get($entity_field_name)->isEmpty() || $translation->hasSourceFieldChanged())) {
$translation->set($entity_field_name, $media_source->getMetadata($translation, $metadata_attribute_name));
}

In the meantime we've copied code from core and manually fixed it in our module. Once this issue is fixed, it'll make that code and our metadata sync controller redundant. So +1 to prioritizing a fix here!

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

Looks like a bit of linting work is needed to get tests to pass. It'd be good to fix those here.

Not sure why the actual phpunit tests aren't executing... will have to dig into this more. (or someone else can look) .. maybe gitlab stops phpunit testing if linting fails?

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

MemcacheBackendUnitTest was moved/renamed to the MemcacheBackendTest in 2018 by Damian.

https://git.drupalcode.org/project/memcache/-/commit/f30543ba2d991e5fd06...

There isn't any issue associated with it, but I'm guessing it had to do with the bootstrapping of memcache. Interestingly its still referencing GenericCacheBackendUnitTestBase -- so I'm not sure why it was moved out of a unit test.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

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

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

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

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

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

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

What version of Acquia Search are you using? The code from this patch was commited and released in version 3.1.3

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

Typically this error is probably occuring if cron is misconfigured (or not configured), or during a migration or other process where lots of invalidations are happening at once.

To counter this edge case, I added a new flag to the state system called purge.dangerous -- if you set this in settings or with drush sset purge.dangerous TRUE then you should be able to have the purger run with over 100,000 items in the queue.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO
πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

Looking at the initialize code, it seems the problem is if blacklist is null instead of an array. By setting it to an empty array should fix this issue without causing a regression that this code initially solved (Line 94 of CacheTagsQueuer.php was throwing invalid key in certain situations)

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

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

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

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

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

These were converted at sometime in the last 3 years to PHPunit

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

These are random Ids, and has nothing to do with encryption or security. No need to fix.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

I don't think purge will recommend specific environments, since ddev, docksal, and lando all have varnish configurations.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

Dup of https://www.drupal.org/project/purge/issues/2952277 ✨ Minify the cache tags sent in the header Needs work

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

Totally agree. The ui itself is different than updating the existing settings in Drupal. Fixed!

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO
πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

This seems specific to your site configuration? Any other information or steps to reproduce?

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

This only requires a cache refresh, which was added as a database update to purge.

πŸ“Œ | Purge | Fix failing HEAD
πŸ‡ΊπŸ‡ΈUnited States japerry KVUO
πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

Added a few more with the drush commands moving to purge core. Fixed!

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

Fixed!

Note: the drush queues will be automatically enabled if you have drush_purge enabled now. If you don't, you can manually enable the plugins within the purge ui.

πŸ‡ΊπŸ‡ΈUnited States japerry KVUO

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

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