🇺🇸United States @danflanagan8

danflanagan8 → changed the visibility of the branch 3488780-formatter-wsod to hidden.

I created 🐛 ALA field formatter causes WSOD Active

I'm setting the status of this issue to Fixed since the MR has been committed. Let's focus on the new issue.

danflanagan8 → created an issue.

Even a basic attempt at testing this code would have shown that is fails catastrophically.

The website encountered an unexpected error. Try again later.

TypeError: Drupal\ala\Plugin\Field\FieldFormatter\AdvancedLinkAttributesFieldFormatter::_construct(): Argument #1 ($token) must be of type Drupal\Core\Utility\Token, string given, called in /app/docroot/core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php on line 40 in Drupal\ala\Plugin\Field\FieldFormatter\AdvancedLinkAttributesFieldFormatter->_construct() (line 50 of modules/contrib/ala/src/Plugin/Field/FieldFormatter/AdvancedLinkAttributesFieldFormatter.php).

The dev branch is currently unusable.

danflanagan8 → created an issue.

Looks lik the LB failure is a known rando:

see 🐛 [random test failure] LayoutBuilderBlocksTest::testBlockPlaceholder failing Active or 🐛 [random test failure] Drupal\Tests\layout_builder\Functional\LayoutBuilderBlocksTest::testBlockPlaceholder Active

I think I have the Unit test fixed.

Closing a dupe of 🐛 [random test failure] LayoutBuilderBlocksTest::testBlockPlaceholder failing Active

This is being tracked in the meta also: 🌱 [meta] Known intermittent, random, and environment-specific test failures Active

I just ran into this in an issue I was working on.

There were a couple failures:

Layout Builder Blocks (Drupal\Tests\layout_builder\Functional\LayoutBuilderBlocks)
┐
├ Behat\Mink\Exception\ResponseTextException: The text "Placeholder for the "The block label" block" appears in the text of this page, but it should not.
│
│ /builds/issue/drupal-3091671/vendor/behat/mink/src/WebAssert.php:907
│ /builds/issue/drupal-3091671/vendor/behat/mink/src/WebAssert.php:312
│ /builds/issue/drupal-3091671/core/modules/layout_builder/tests/src/Functional/LayoutBuilderBlocksTest.php:219
┴

and

Drupal\Tests\views\Unit\Plugin\argument_validator\EntityTest 0 passes 1s 1 exceptions
FATAL Drupal\Tests\views\Unit\Plugin\argument_validator\EntityTest: test runner returned a non-zero error code (2).
Drupal\Tests\views\Unit\Plugin\argument_validator\EntityTest 0 passes 1 fails

I pushed up a fix that uses the same kind of funky technique as #2853592: Cacheability metadata can't be set from within argument default handlers. → .

I added a fail test that exposes the bug. Now we just need to fix it!

Great! I'm going to change this to a support request and mark it fixed so that we can get credit to @jonathan_hunt.

Cheers!

The other issue is some protective code that's added to Views UI. I think that prevents a WSOD when editing a View in Views UI, and it may prevent saving a misconfigured View.

However, there are no update hooks or the like to fix existing Views configuration that has extra spaces. Nor would that issue fix any extra-space bugs that would be the result of a bad template or something like that.

And it sounds like that's where your problem crept in, right? Trailing spaces in twig (either in the View config or in the template)?

If I'm understanding correctly, then there is no bug in the viewfield module. This module in no way allows configuration like "Output this field as a custom link". The way I see this is that when a misconfigured View is referenced by viewfield there can be trouble. But that's a problem with the View configuration, not a problem with viewfield.

Am I wrong? Should we close this as "Works as Designed"?

I spun up umami with D10.3.8 and I'm not having any luck triggering the error. I made a content type with a viewfield and added a few of those nodes. Then I made a view of those nodes. I used [node:nid] as a argument for one of the viewfields. Still no errors.

While the txt file in #8 is non-unhelpful, it's only so useful since there are many other dependencies and I can't import that config.

I don't think there's anything I can do until there's either:

1. More of a backtrace with the error

or

2. Steps to reproduce starting with a new install

Thanks for the report.

I just ran the tests on current and previous majors and unfortunately did not get any fails.

https://git.drupalcode.org/project/viewfield/-/pipelines/340030

Darn. Some fails would have helped.

I created an upstream issue (I guess?) hoping for a new build script that generates a dist js.

https://github.com/CMSgov/hpt-validator/issues/62

And there it is!

Merged! Nice work, all!

danflanagan8 → made their first commit to this issue’s fork.

danflanagan8 → created an issue.

Back to NW just to get that logo in an MR. Thanks!

I think #11 looks great, @sourojeetpaul.

If you put that in an MR, I'll happily merge it. :)

Hi @sourojeetpaul!

I really like this part of your new logo:

What if we make that the entire logo? That would look great at all sizes. It's also a good representation of the module, right? We're looking into the database and seeing the relationships.

I'm going to set back to NW, but I think we're really close to a good solution now!

MR and patch both look good now!

The patch looks good. I searched the 1.1.x branch for all occurrences of const string and there were exactly 8 of them. All are resolved in the patch.

One of them got missed on the MR though. Back to NW for that.

My team just ran into this. It's not an issue on the 2.x release, luckily.

I think you nailed it, @kristiaanvandeneynde:

I think this shows why we should have more classes in core marked as internal and/or final.

Agreed! I was kind of surprised wasn't internal.

Thanks for your comments to me and your work on the issue. Cheers!

Thanks, @kristiaanvandeneynde.

Am I really allowed to pass four arguments to a constructor that accepts only 3 arguments? We were passing the EntityTypeManager as the fourth argument rather than relying on the already existing BC layer for that fourth argument (lots of work on this class lately!).

I'm still curious about removing a property from the class:

Beyond that, the `$cache` property is removed from PermissionsHashGenerator. Isn't that pretty big BC no-no kind of stuff?

My extension of PermissionsHashGenerator was using that property and therefore completely broke.

What version of Drupal is this?

My understanding is that a translated string as a category is deprecated but shouldn't result in errors yet.

rajeevk → credited danflanagan8 → .

I'm not sure why we're concerned about how gitlab renders it!

Because that's one of the essential uses of this logo! See the IS:

as well as on drupalcode.org as your project avatar

Thinking of it as an icon is a good idea in my opinion. Here are some effective examples:

Token:

Redirect:

Search API:

And here's one I created for a project I maintain:

The issue for that one shows that I made the misstep of making things too complicated at first as well. 📌 Project Browser: Create a logo for Crossword Fixed

We need to keep it simple.

it also aligns with the module's functionality

I don't know that I agree with this. To me it looks like the logo is for a form builder or something like that. I don't have any great ideas, but I can't say I convinced this is a good choice. I showed another maintainer and he was skeptical as well.

I also think we should keep in mind that this logo doesn't seem to be rendered at 512px very often. On a project page it's 60px wide. And within entity browser it's more like 256px or something. This logo is not great at 60px:

I put in an MR that uses #states to make the display id required when the view id is set. Maybe real validation would be better because #states requires JS, but seriously who is configuring this without JS enabled.

danflanagan8 → made their first commit to this issue’s fork.

If we're being honest with ourselves, no one's going to review this. :(

I'm going to merge as is. It's based on some work we've used extensively on client projects and it has some basic test coverage and it's completely independent of other stuff, so it's not likely to muck any existing code up.

Perfect! Without the fix the updated test fails with:

1) Drupal\Tests\viewfield\FunctionalJavascript\ViewfieldWidgetTest::testSelectWidget
Behat\Mink\Exception\ExpectationException: An option "block_disabled" exists in select "field_view_test[0][display_id]", but it should not.

With the fix it passes. The fix itself looks good to the eye as well. Let's merge.

Thanks!

Thanks for the issue! I'm surprised this isn't a dupe, but I don't see any similar reports in the queue.

I added some test coverage to the MR. Let's see how that runs in Gitlab.

danflanagan8 → made their first commit to this issue’s fork.

Ok, this is definitely a source plugin thing and can't be fixed using Migrate Conditions.

This might require a slightly modified data_parser plugin. I had a project not long ago where I made a modest extension of the Json data_parser to deal with Json that had a slightly different structure than what the Json parser expects:

/**
 * Obtain JSON data for migration...when the JSON response is super flat.
 *
 * @DataParser(
 *   id = "flat_json",
 *   title = @Translation("Flat JSON")
 * )
 */
class FlatJson extends Json {

  /**
   * The standard json parser expects the response to be an array of arrays.
   * This plugin handles a JSON response that is flat array, allowing the
   * response to be treated as a single row for the migration.
   *
   * {@inheritdoc}
   */
  protected function getSourceData(string $url): array {
    $response = $this->getDataFetcherPlugin()->getResponseContent($url);

    // Convert objects to associative arrays.
    $source_data = json_decode($response, TRUE, 512, JSON_THROW_ON_ERROR);

    // If json_decode() has returned NULL, it might be that the data isn't
    // valid utf8 - see http://php.net/manual/en/function.json-decode.php#86997.
    if (is_null($source_data)) {
      $utf8response = utf8_encode($response);
      $source_data = json_decode($utf8response, TRUE, 512, JSON_THROW_ON_ERROR);
    }

    return [$source_data];
  }

}

You might be able to do something similar where you introduce special handling for NULL data.

I'm going to close this issue since it's outside the realm of Migrate Conditions. The Drupal Slack #migration channel would be a great place to ask for additional help here. Good luck and happy migrating!

Hi Ste,
I suspect this is something that would be better handled in your source plugin. What source plugin are you using? What is the error that you are seeing?

I was fortunate enough to meet with the OS team at CMS and ask some questions about the validation stuff! What a nice group of devs.

First, there are no plans to create an api endpoint (security concerns and concerns over huge file sizes). Second, there are no plans to create a php library for validation (they want a single source of truth for validation, which is and will be the typescript library).

So what can we don with the npm package that has the validation as js?

The npm package is here: https://www.npmjs.com/package/hpt-validator

The github for that is here: https://github.com/CMSgov/hpt-validator

Naively, I want to make that npm package a dependency of the Drupal module. The module would then define a validation library that more-or-less is a drupal wrapper for the hpt-validator js. The validation would then happen on the front end, probably during file upload. Perhaps also on page load of the hpt edit form. And maybe after some use action (e.g. button click) on the hpt collection page.

I'm never sure what the current best practice is for js dependencies, though, so I need to research that.

v2 info

weight

Contact fields

cms-hpt update

v2 settings form

file clarification

add links

add explanation section

fix images

Added intro and example

I wrote a blog post about the fun I had here

https://blog.horizontaldigital.com/add-icon-your-custom-field-type-drupa...

This broke a site where we extend PermissionsHashGenerator.

When calling parent::__construct($private_key, $cache, $static, $entity_type_manager); it goes WSOD because the PermissionsHashGenerator constructor got changed to accept three arguments instead of four. Beyond that, the `$cache` property is removed from PermissionsHashGenerator. Isn't that pretty big BC no-no kind of stuff?

I see there was a conversation about BC breaks (https://git.drupalcode.org/project/drupal/-/merge_requests/4494#note_284734) in contrib in at least on context, but I don't see any conversation around several other potential BC problems.

Tagged a new release

https://www.drupal.org/project/acquiadam_asset_import/releases/2.0.0-beta2 →

Thanks for the heads-up, @rajeshreeputra!

I'll merge this and tag a new release. Cheers!

danflanagan8 → created an issue.

Change step number

Remove outdated step numbers

I'm having fun and making lots of progress! I need to add tests. I also think I'm going to do this on a new 2.x branch because there are a huge number of updates and, though they aren't necessarily disruptive, I want people to pay attention and be careful during the update.

Crediting @allie micka who conceived this and wrote the first version of what I'm about to post.

danflanagan8 → created an issue.

We need this to be an MR before we move any further since we can't run tests on patches anymore.

Thanks, @waspper. That looks correct. If you create an MR out of the patch, I'll merge it.

D11 has been out for ages. Nothing new is going to come through. Closing.

danflanagan8 → created an issue.

> Could be our view isn't getting cached in any case for some other reason.

Yeah, I was wondering myself why the tests didn't fail without this. The views in the test module all have caching configured correctly, from what I can tell. There's at least some chance that the presence of the "content type" filter on the View would result in the config:node_type_list cache tag getting added to the page. That's my best guess, but just a guess.

Thanks for making the change!

That doc doesn't really describe this use of getCacheTags unfortunately. This is the usage where we're saying, "Cache the result of this until these tags are invalidated." If any node type has its configuration updated, it's possible that the list of micronode types has changed and so we can no longer trust the cached results of the filter.

That MR looks correct to me, @justcaldwell.

I don't know if it's really legal for me to RTBC this. I guess I'm asserting that the MR contains the code I posted on the other issue and that you tested on the other issue.

> All credit goes to @danflanagan8 for the fix.

Don't forget that I should also get all the credit for the regression!!!

I updated the query method locally to query more like the bundle filter and I got the tests to pass:

  /**
   * {@inheritdoc}
   */
  public function query() {
    $this->ensureMyTable();
    $micronode_bundles = array_keys(micronode_get_node_types(TRUE));
    if ($this->value) {
      // Restrict to micronodes.
      if ($micronode_bundles) {
        $this->query->addWhere($this->options['group'], "$this->tableAlias.type", $micronode_bundles, 'IN');
      }
      else {
        // If there are no micronode bundles views should return nothing.
        // Modeled after Drupal\Core\Database\Query::alwaysFalse().
        $this->query->addWhereExpression($this->options['group'], '1 = 0');
      }
    }
    else {
      // Exclude micronodes.
      if ($micronode_bundles) {
        $this->query->addWhere($this->options['group'], "$this->tableAlias.type", $micronode_bundles, 'NOT IN');
      }
    }
  }

I'm trying to figure out the best way to push this up for a new MR.

I'm no expert in SQL queries, but maybe the updated where clause leads to issues at scale?

Neither am I, though as I was making this change I did worry that "NOT LIKE" was going to be slower than "LIKE". I was hoping that worry was unfounded but perhaps I was right to be concerned.

Since I'm kind of nuts I had been thinking about how to rework this filter even before the comment from @justcaldwell. It occurred to me that it might make sense to copy off of whatever the normal node type filter does in the query. We'd do a call to micronode_get_micronode_types or whatever to get the content types we need to include. I'm not sure if xtending that filter makes sense, given all the extra configuration options and stuff that filter has. Maybe though.

Yay! After some waiting for the old cached version to go away (I presume?), this looks really good!

This works as designed. The idea is that items are only created in the queue if the queue is already empty. Otherwise we leave things alone and let all the queue items get processed before we consider adding new items to the queue.

> Personally, I think of it the second way.

Gotcha. Re-reading the IS, I'm convinced that's what's being asked for anyway.

> A more important consideration is that the plugin should behave the same way in Drupal 10.2 as it does in 10.3 and 11.0.

It should be tested manually, but the latest looks good to me. It's right to return $value instead of NULL, which you must have realized after #37

Leaving at NR for the T part of RTBC, as you say.

This is great stuff. Thanks, @benjifisher!

I got a bee in my bonnet and pushed something up for this one.

An install and update hook would probably be sufficient, but I also wanted to address the bug related to "Is not equal to" and generally try to improve the filter's UX.

The MR addresses the problem without an update hook. And here's what the form looks like afterwards.

Definitely clearer than it is now (and more functional) though I wouldn't claim to have picked the best wording possible.

Here's what it looks like now, for reference.

A little confusing. And the "Is not equal to" isn't functional.

I just noticed this on the project page:

Views will only recognize nodes whose types have this flag at either TRUE or FALSE, but on existing types this will be NULL unless they are re-saved.

I put in an MR with the hook and some test coverage. It looks like tests aren't set up to run on Gitlab for this project, but the MicronodeViewsTest passes locally for me.

danflanagan8 → created an issue.

I'm going to put in a real MR with some test coverage, but the hook is going to look something like this:

/**
 * Remove types from exposed filters when 'Is Micro-content' filter is in use.
 *
 * Implements hook_form_views_exposed_form_alter().
 */
function micronode_form_views_exposed_form_alter(&$form, FormStateInterface $form_state, $form_id) {
  if ($view = $form_state->getStorage()['view']) {
    $filters = $view->getHandlers('filter');
    if (isset($filters['micronode_is_microcontent'])) {
      $return_microcontent = $view->display_handler->getHandler('filter', 'micronode_is_microcontent')->value;
      $allowed_bundles = array_keys(micronode_get_node_types($return_microcontent));
      foreach ($filters as $id => $definition) {
        if (isset($definition['exposed']) && $definition['exposed'] && $definition['plugin_id'] == 'bundle') {
          foreach ($form[$id]['#options'] as $key => $value) {
            if (is_string($value) && !in_array($key, $allowed_bundles, TRUE)) {
              unset($form[$id]['#options'][$key]);
            }
          }
        }
      }
    }
  }
}

This is a refactored version of something originally conceived of and written by the @allie micka. We need to make sure to give credit if and when it comes to that! I love this feature, though, and hopefully it's not too hard to sell. :)

danflanagan8 → created an issue.