search

ESAPI / esapi-java-legacy

ESAPI (The OWASP Enterprise Security API) is a free, open source, web application security control library that makes it easier for programmers to write lower-risk applications.
https://owasp.org/www-project-enterprise-security-api/
Other
610 stars 368 forks source link

Change skin for mvn site report to use fluido #731

Closed davewichers closed 2 years ago

davewichers commented 2 years ago

The 4.0.0-Mx version of the -site plugin is broken if you don't specify a recent version of a skin it supports. So I am upgrading that plugin, specifying the skin version in the pom, and also created a custom site.xml based on AntiSamy's that references that skin. I also turned off the tag reports since they are all broken currently.

kwwall commented 2 years ago

'target/site/property-updates-report.html' is still broken (missing trailing '}'. And not really sure that I like the image at the top of the site reports. Mostly just takes up room. I had already had updated to '-M3' version of the plugin but hadn't pushed yet and if it doesn't work without a skin, I obviously hadn't run 'mvn site', but I don't see any place where the '-M2' version was failing that '-M3' also isn't still failing. (E.g., the tag library documentation, which I what I think we miss most still isn't working as you mentioned in a private email.)

Anyway, if you create a GitHub issue and describe what reports this actually broken with it (so I can reference it in the next release notes), I will merge this.

BTW, you would think they would have had required the site.xml file to go under 'src/main/assembly' or maybe under 'src/main/resources' rather than 'src/site'. Adding more directories for no good reason (versus using an existing one) just means it's yet another place to forget about things. Not your fault, I know, but 'src/main/assembly' would have been better IMO.

kwwall commented 2 years ago

Oh, one more thing. if we're going to have a logo on the 'mvn site' reports, I'd prefer to use the ESAPI logo instead (attached), scaled down of course. esapi-logo

davewichers commented 2 years ago

Tag pages were broken or blank before if I recall. I see no reason not to merge.

On Tue, Jul 26, 2022 at 9:28 PM Kevin W. Wall @.***> wrote:

'target/site/property-updates-report.html' is still broken (missing trailing '}'. And not really sure that I like the image at the top of the site reports. Mostly just takes up room. I had already had updated to '-M3' version of the plugin but hadn't pushed yet and if it doesn't work without a skin, I obviously hadn't run 'mvn site', but I don't see any place where the '-M2' version was failing that '-M3' also isn't still failing. (E.g., the tag library documentation, which I what I think we miss most still isn't working as you mentioned in a private email.)

Anyway, if you create a GitHub issue and describe what reports this actually broken with it (so I can reference it in the next release notes), I will merge this.

BTW, you would think they would have had required the site.xml file to go under 'src/main/assembly' or maybe under 'src/main/resources' rather than 'src/site'. Adding more directories for no good reason (versus using an existing one) just means it's yet another place to forget about things. Not your fault, I know, but 'src/main/assembly' would have been better IMO.

— Reply to this email directly, view it on GitHub https://github.com/ESAPI/esapi-java-legacy/pull/731#issuecomment-1196167375, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGFWBOQBXBDEMMLPW73GD3VWCGFFANCNFSM54TQ4EBA . You are receiving this because you authored the thread.Message ID: @.***>

kwwall commented 2 years ago

Please create a GitHub issue, assign it to yourself, and then mention this PR in it and I'll do the merge. I just don't like to have commits / PRs not associated with GitHub issues (except for minor bookkeeping issues during releases, like removing '-SNAPSHOT', etc.) because I document the GitHub issues that were closed in release notes, not the specific PRs or commits and IMO, that's a more useful synopsis than just (say) looking at a change log.

kwwall commented 2 years ago

On a previous episode, @davewichers wrote:

Tag pages were broken or blank before if I recall. I see no reason not to merge.

These tag pages are still broken. The only difference I see is that the indexes for:

no longer appear in the sidebar navigation panel so you just don't notice. I will write up a GitHub issue, but aside from some minuscule issues such as a messed up copyright notice (the year appeared as "Copyright © ${currentYear} ..."), I really don't see what was fixed. So to me this PR just looks like it changes the look-and-feel (admittedly, to one that appears a bit slicker) but really doesn't fix anything. So I need to know what to put into the the GitHub issue that I will create. So, I don't even know if this would be considered primarily a 'bug' or an 'enhancement' and thus I need some help in knowing what to put into the GH issue.

Also, note that with this PR merged, there will also a be downside of 'mvn site' no longer generating that sidebar navigation index which means that I had to create GitHub issue #733 so I wouldn't forget about the tag documentation not working. (Out-of-sight, out-of-mind.)

davewichers commented 1 year ago

I don't have time/interest in creating an issue. Please do so yourself so you can then reference it in your acceptance of this commit. I already spent an hour+ on this.

-Dave

On Tue, Jul 26, 2022 at 9:28 PM Kevin W. Wall @.***> wrote:

'target/site/property-updates-report.html' is still broken (missing trailing '}'. And not really sure that I like the image at the top of the site reports. Mostly just takes up room. I had already had updated to '-M3' version of the plugin but hadn't pushed yet and if it doesn't work without a skin, I obviously hadn't run 'mvn site', but I don't see any place where the '-M2' version was failing that '-M3' also isn't still failing. (E.g., the tag library documentation, which I what I think we miss most still isn't working as you mentioned in a private email.)

Anyway, if you create a GitHub issue and describe what reports this actually broken with it (so I can reference it in the next release notes), I will merge this.

BTW, you would think they would have had required the site.xml file to go under 'src/main/assembly' or maybe under 'src/main/resources' rather than 'src/site'. Adding more directories for no good reason (versus using an existing one) just means it's yet another place to forget about things. Not your fault, I know, but 'src/main/assembly' would have been better IMO.

— Reply to this email directly, view it on GitHub https://github.com/ESAPI/esapi-java-legacy/pull/731#issuecomment-1196167375, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGFWBOQBXBDEMMLPW73GD3VWCGFFANCNFSM54TQ4EBA . You are receiving this because you authored the thread.Message ID: @.***>

kwwall commented 1 year ago

@davewichers wrote:

I don't have time/interest in creating an issue. Please do so yourself so you can then reference it in your acceptance of this commit. I already spent an hour+ on this. … -Dave

Huh? I already created the issue for this (#734, no closed) and already merged this PR. Looks like you are maybe responding to old emails from your inbox??? Anyhow, this is all taken care of.