Florida
Account created on 25 April 2007, about 18 years ago
#

Merge Requests

More

Recent comments

🇺🇸United States swirt Florida

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...

🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

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)

🇺🇸United States swirt Florida
🇺🇸United States swirt Florida
🇺🇸United States swirt Florida
🇺🇸United States swirt Florida
🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

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.

🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

Nicely done @skyriter. I like this new pattern.

🇺🇸United States swirt Florida

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.
🇺🇸United States swirt Florida

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.

🇺🇸United States swirt Florida

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.

🇺🇸United States swirt Florida

@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.

🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

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?
🇺🇸United States swirt Florida

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 .

🇺🇸United States swirt Florida

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.

🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

@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.

🇺🇸United States swirt Florida

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.

🇺🇸United States swirt Florida

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`

🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

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:

🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

@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."

🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

I believe this is ready for review.

🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

Looks good, and merged.

🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

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.

🇺🇸United States swirt Florida

swirt created an issue.

🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

This would be in addition to already adding
field_heading
field_weight

🇺🇸United States swirt Florida

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"
🇺🇸United States swirt Florida

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.

🇺🇸United States swirt Florida

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.

🇺🇸United States swirt Florida

@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.

🇺🇸United States swirt Florida

swirt created an issue.

🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida
🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

swirt created an issue.

🇺🇸United States swirt Florida
🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

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.

🇺🇸United States swirt Florida

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.

🇺🇸United States swirt Florida

@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.

🇺🇸United States swirt Florida

Thank you Nidhi27. I will test this out soon.

🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

Thank you for solving these issues.

🇺🇸United States swirt Florida

Thank you skyriter for this fix.

🇺🇸United States swirt Florida

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

🇺🇸United States swirt Florida

Thank you for this contribution.

🇺🇸United States swirt Florida

Thank you skyriter this is a helpful and appreciated contribution.

🇺🇸United States swirt Florida

Thank you for getting the phpstan to be happy.

🇺🇸United States swirt Florida

Fantastic spellerizing. Thank you for this contribution.

Production build 0.71.5 2024