🇺🇸United States @swirt

Probably want to force the module weight to make sure this fires AFTER sitewide_alert
https://www.drupal.org/docs/develop/creating-modules/understanding-hooks... →

Looks like maybe we could just implement this hook and then unset $page_top['sitewide_alert']

That lives in the base module. Can we override it by putting it in our module. ( I would not think so, but I have no proof)

swirt → created an issue.

Thank you for your work on this @tom konda. It is appreciated.

Thanks @nidhi27. I added some AC's

Unless you have a site set up with some existing migrations, I suggest using migrate_example which is a submodule of migrate_plus. That can give you some migrations that should then show up as documentable.

Thank you @Nidhi27, I added some AC's to make it clearer.

Hi Nidhi27. No this is not resolved yet. I added some Acceptance Criteria as well.

Hi @nidhi27 I added acceptance criteria. Let me know if you have any questions.

Nicely done @skyriter. I like this new pattern.

swirt → created an issue.

swirt → created an issue.

Good question. Every img tag discoverable within the "content" will show in the report regardless of any alt or title attributes. Usually, true "decorative images" are on templates directly and are not discoverable as content. However, if someone puts a decorative image in the WYSIWYG it will appear in the report.

I think there are a few reasons to have them in the report:

1. An audit is not helpful if it lacks the ability to surface all things.
2. It could be vital to be able to filter for all "decorative" images so they can be examined to see if an editor was being disingenuous and taking the easy way out by labeling them decorative.
3. Yesterdays decorative may need to become tomorrow's content
4. alt attributes on a given media image are the same everywhere they are used, so in the original use of the image it might have been decorative, but in a subsequent instance it might have been intended as content.

It will report every img tag that it can find. The filters should/would allow you to restrict it to only things with errors or warnings.

I think I understand. Are you saying that config/install in uswds_alert will squash the settings (or the config/install in sitewide_alert)?

I think there is a possibility. There is a chance of one thing clobbering the other. Uncharted territory for me. We probably have to do some testing with a site that has a mock existing setup of sitewide_alert and then see what happens when uswds alert is installed after that.

@gxleano thank you for getting this resolved. If you have time, please remove my contrib credit on this. I did not do anything on this issue other than change its status. I appreciate the credit, but would rather earn it legitimately.

This went out with release 1.0.35 →

This went out with release 1.0.35 →

This went out with release 1.0.35 →

This went out with release 1.0.35 →

This went out with release 1.0.35 →

This went out with release 1.0.35 →

This went out with release 1.0.35 →

This went out with release 1.0.35 →

Thank you @nidhi27 for your work on this. It is a great addition to the module's capabilities.

swirt → created an issue.

I'm not totally sure the "chip" idea (where I list the filters I searched for directly above the results) is possible, but I thought having just a quick view of what I filtered by could be useful to the end user.

The chip idea is a nice improvement in general to Views filters, but I don't see a contrib module that does that. (There might be one, I just haven't seen it). Even if there was one I don't think we'd want to include it because it would likely force chips on to all Views, and would be kind of opinionated for this module to add. So that change is pretty far out of scope I think.

will all the data in the table populate even if I don't filter by it? Or does it only populate when no filters/that specific filter is employed?

Views in Drupal usually start fully populated and then filter from there. Having a View start as empty and then populate when filtered would create a couple concerns.

1. It would change the nature of the "report" to being acting more like a "search", which would be an anti-pattern.
2. How would you ever get a complete / full report if the only way to make it populate is to add at least filter?

I think we might have veered off a bit here and crossed areas of concern here. This issue is about the admin screen, The other issue is for the Report ✨ UX design for alt text report Active .

skyriter, yes exactly.

I think DavidmPickett's proposal is superior by splitting it up into 2 fields (style and type). My main concern is largely based on fear of not being able to fully anticipate existing scenarios of people already using sitewide-alert that are then just adding this module on top of it. We are already courting trouble by looking at how to interweave our config into existing config. This plan adds another layer on top of that.

If this were a standalone module I would have no concerns about Dave's content model as it is really solid.

Yes, makes sense. This alone does very little. But does a little. I will work on fixing the failing tests to get this completed.

@catch I took your suggestion on this MR and made it use a shorter hash https://git.drupalcode.org/project/drupal/-/merge_requests/11787

@cliefen, These are all good questions, and I do not pretend to be a security expert but my current thinking on it is this:

That’s all on the assumption that this is a security improvement which I’m doubtful about.

Version disclosure is labeled as a low risk but is serious enough that Drupal does not reveal its minor version, Only the major.

<meta name="generator" content="Drupal 7 (https://www.drupal.org)" />

<meta name="Generator" content="Drupal 10 (https://www.drupal.org)" />

If it is serious enough that Drupal does not reveal itself, then Drupal should probably show the same level of care to libraries and not reveal their specific version.

In fact, don’t these libraries declare their versions within their code, defeating this proposed protection?

Many do. Example: jQuery.fn.jQuery will reveal its own version. If the library wants to reveal its own version, that is the library's choice. However, I think that should remain the choice of the library's maintainers. Drupal should not be the library's loose lipped friend that blabs the secret. I think this falls into the same reason why we routinely make the composer.lock and composer.json of our sites return an access denied.

While the Tugboat is available, this url should return fine
https://mr11806-gs7qqwaidh3locbz5vsd5vt5golwwmyn.tugboatqa.com/index.php

And this one should result in 404
https://mr11806-gs7qqwaidh3locbz5vsd5vt5golwwmyn.tugboatqa.com/index.phpboo

I created a new branch and PR ^^ for an .htaccess approach to the problem. It is simpler, and since most sites customize their .htaccess anyway, this would allow them to alter it without really needing to patch core.

I am running into this being flagged by Invicti/Netsparker scan because they try to hit nonsensical pages like

• /index.php.
• /index.php.?hTTp://r87.com/n
• /index.phpI-dont-exist
• /index.php.php/some/path

And Invicti objects to getting a 500 response instead of a more appropriate 404. I agree with desoi, acbramley and alexpott who suggested this should be a 404. Sites with a 500 error look broken and it should not be this easy to make a Drupal site look broken.

alexpott indicated there was a possible problem with 404 ing in this case. I wonder if instead of trying to handle this at the php level and replacing the thrown exception with a 404, if it would be cleaner to have a well crafted rule in .htaccess that handles the non slash characters after `index.php`

Fantastic work @nidhi27. This works better than I imagined it would. Very well done and thank you for this contribution.

LOL. Interesting. I don't think that would security risk though as CSS rarely has exploitable issues. At least not that I am aware of. :shrug:

I updated the steps to reproduce to be more specific about when and where they are exposed.

@cliefen Good question. Two different answers:

1. Even with javascript aggregation enabled, when you hit update.php or install.php, it seems they do not use aggregation and the individual script tags are present, which is where the versions are visible.
2. We have aggregation enabled on our stage an prod environments, so on all pages other than update.php or install.php it is not a full problem there. However we do have aggregation off on our dev environment so that is easier to debug js issues. And on our gov't projects all three environments are scanned due to Zero Trust mindset.

It is nice that aggregations solves it for most requests, and the Drupal recommended approach is to use aggregation, I don't think it should be a security requirement. There is nothing on the performance UI that says "use JS aggregation or your site will be more at risk."

Bonus points for providing visitor access to make it open :)

I believe this is ready for review.

This is no longer blocked and should be ready to roll.

Looks good, and merged.

I would look toward rendering with Single Directory Components. One per block. Ideally they would also be useable as a decoupled component.

If there is interest in this actually being merged, let me know and I will rework the failing asset tests to account for this change.

swirt → created an issue.

Thank you so much @Nidhi27 and @skyriter for your work on this.

This would be in addition to already adding
field_heading
field_weight

Nice idea. I don't think we should be adding any entity as we are leveraging the sitewide_alert entity. I could see adding fields to that entity if we wanted to account for the options:

• a checkbox to allow choosing "no icon"
• a checkbox to allow choosing "slim"

Your summary is right.

Why is would this type of alert a part of this module, given its purpose to provide USWDS alerts and site alerts?

It is for the case where sites are using the sitewide_alert module for other things. Installing uswds_alert will render the default block provided by sitewide_alert unusable because it will blindly display all events. So we need to provide a replacement block for non USWDS style alerts.

Another consideration after someone figures out how to implement Views data for a config entity is to provide better documentation so that module developers creating config entities can do a better job of including it themselves.

@cosmicdreams I like this refined approach (disclaimer: I only understood maybe 80% ;) ) . If config entities have the ability to provide Views data it makes sense to leverage that as much as possible.

One challenge I see is doing it in a way that would not break any existing Config Views. Hopefully the underlying architecture change would be un-noticeable to the casual user.

swirt → created an issue.

I merge MR1 so this should be clear to add the dependencies to the composer json and move on.

swirt → created an issue.

Hi Nidhi27, sorry about that. I just added some Acceptance Criteria. Let me know if you have questions.

@nidhi27, Great question. I just added some detailed Acceptance Criteria to the issue description. Let me know if you have other questions.

swirt → created an issue.

This looks good. I merged this to clear the way for the composer.json.

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

I wouldn't close it, just add a commit to it.

You might want to include the dependency on https://www.drupal.org/project/sitewide_alert → and add the module.info file as part of this.

Is this the image title if the media library is used?

Yes and no. Image fields can have their own title field just like they have an alt field. Then when they are rendered, this populates the title attribute and alt attribute on the img tag. Image title column will surface the img title attribute if there is one.

I suggest swapping the locate of 'Image title' and 'File' columns so that it is easier to compare title and alt as many times those can be too similar.

Do these refer to any rule that was applied...

Yes this would list any rules/warnings that were violated. It would likely display them as an html list.

@adriancooke. This is great, thank you.

The report section makes sense and is completely attainable. The second section though for the admin settings is not really attainable.
The filters and the columns on the report are entirely controlled by a sub system in Drupal called "Views". Views gives any admin the ability to alter the View to move/add/remove filters, add/remove/re-arrange columns and much more. Any site will have the capability of modifying the report to meet their needs with Views. There is no reason that we need to rebuild that functionality with Alt Text Validation module's admin.

Thank you Nidhi27. I will test this out soon.

Well that is beyond opinionated. Outlawing a perfectly fine spelling variant is creating a problem that does not exist in nature. 😀

Yay we have full passing tests!!! Thank you for this contribution.

Thank you for solving these issues.

Thank you skyriter for this fix.

Good idea. It is merged now so you should be clear.

Thank you for this contribution.

Thank you skyriter this is a helpful and appreciated contribution.

Thank you for getting the phpstan to be happy.

Fantastic spellerizing. Thank you for this contribution.