@seeduardo

While I can confirm the code in #5 does work on D10.3, it is simply a repetition of what's in the merge request mentioned in #3. I agree with dqd in #4 that the change could be rewritten, but the code in that comment isn't quite right and would be a breaking change, so I've tweaked it in the patch attached here, which does work for me.

seeduardo → created an issue.

This has been tested by running the SG API module as it already was on a local site and then loading the changes onto the same site to compare - all looks good.

Tested and all good.

Ok appears the solution is robust enough, thanks so much for all testing and comments and others who helped with this - merging and closing now.

Ready for review, thanks anyone and everyone.

seeduardo → created an issue.

Looks all good, thanks @arcaic.

Many thanks for your comments @vishal.kadam. You will note as at comment #17 here, that PHPCS has been stringently run on the Site Guardian code, and according to stricter standards than normally required. None of the issues you mention have ever come to light that way, and running the sniffer again just now, they still don't. Nevertheless, I agree that the class constructor docblocks you mention are contemporarily extraneous, and not necessary in our case anyway, so I have removed them altogether.

However, I was more confused by your comment that 'Function and method declarations are written on a single line.' Are you still referring to the class constructors from which you pasted code? If so, I'd have to say that the coding standards are more ambiguous on this matter than you are.

For instance, I could stick all the arguments for the SiteGuardianController constructor (as per your second code block example) onto one line, but then PHPCS would indeed throw up an error of exceeding the 80 character limit. The coding standards on function declarations → actually don't mention the single line issue you're talking about at all, and whilst it is true that the standards section on line length → does nominally permit longer function definitions to exceed 80 characters, it doesn't say they should or have to - it is left to interpretation.

Above, in comment #16 here, @apaderno is far more stringent than this: while Drupal.org's wording states "In general, all lines of code should not be longer than 80 characters.", he instead, in this very project application, writes that "the Drupal coding standards say text lines must not exceed 80 characters, so longer lines is bad, not good" (my emphasis added), which would certainly contradict what I interpret as your somewhat tacit prescription of an exceedingly long class constructor declaration with all its arguments on a single line.

Indeed, you'll note that there are instances in Core itself where function declarations are not just spread across multiple lines but also decidedly more convoluted than mine, for example in the AttributeClassDiscovery plugin, whose constructor declaration (at time of writing) starts at line 45 but runs down to line 49, and it's arguable that perhaps it's best that it does.

For all these reasons, and above all else for what I see as plain old readability's sake, I'm electing to leave any multi-line constructor declarations mentioned in your comments just as they are.

Many thanks once again @vinaymahale and @apaderno - I have now made changes to the project in line with recommendations, as well as a couple of related/corollary changes.

Thanks @arcaic, I've tested this now and it all looks good. Merging forthwith.

Thanks @arcaic, I've tested this now and it all looks good. Merging forthwith.

Just reporting that the patch at comment #2 fixes the generic incompatibility complaint on a site's D10 Upgrade Status page:

Please merge when you can, many thanks in advance.

Just reporting that the patch at comment #2 fixes the generic incompatibility complaint on a site's D10 Upgrade Status page:

Please merge when you can, many thanks in advance.

Thank you all @vinaymahale, @dunx and @apaderno. The PHPCS issues reported in this thread have now been addressed, as well as others noted by using more stringent set of standards (interesting exercise to compare these).

All small things, but important and reasonable. Looks good to me, thanks @the_g_bomb, merging.

I can confirm that the patch at #101 is working correctly for me on Drupal 9.5.10, without the issues mentioned in #106 - thanks for that, and see below when Body field set to 5 rows, for an example:

All changes (including those being "considered" in the description above) have now been included in Site Guardian 1.0.1. Thanks @i-trokhanenko and to all who contributed.

@apaderno, @Rassoni, @daceej - thanks to you all for your comments. The Site Guardian team raised tickets for them all and we have now been through the process of making all the changes necessary, please do refer to the SG issue queue as required. I therefore submit this security advisory coverage application for further review here once again, now that the above issues have been addressed. Many thanks in advance!

Looks fine to me too, thanks both - merged.

The change in patch at #2 has been included with a raft of other D10 changes elsewhere, so not merging here but leaving the issue open so Drupal Rector can continue to do its thing.

I would like to politely suggest that this issue please be reopened.

The changes made in this issue, merged into version 3.4.0, meant that when we upgraded from version 3.3.2, while Admin users were totally unaffected, non-Admin users could no longer log into the site, which was in fact broken for them at that point ("The website encountered an unexpected error."). The logs show that this was due to a PHP error:

UnexpectedValueException: base:devel/taxonomy_vocabulary/<vocabulary-name> has no corresponding route. in Drupal\Core\Url->getRouteName() (line 567 of /core/lib/Drupal/Core/Url.php).

Indeed, this error didn't occur on versions of the site where the Devel module was already enabled, and cycling between enabled and disabled on any server in pre-production essentially served as a switch to turn the above error off and on at will. However, as we don't want Devel enabled everywhere for performance (and other) reasons, this was only a temporary solution.

I narrowed the problem down further to the access check being done in the lines surrounding the diff on this merged issue, and simply reversed the changes therein - the problem is now gone for us whether Devel is enabled or not, but of course, this is at the expense of the desired outcome of the entire issue raised here. This doesn't seem to affect us for now but is obviously of relevance to you and perhaps to other people using the module, suggesting the main issue here has not really been fully tested and/or resolved yet.

Thanks so much for this useful input @i-trokhanenko - I would tend to agree, but we can discuss further first if you like @the_g_bomb.

I tried this out myself and looks good. Hitting an incorrect endpoint 10 times means even if the 11th attempt is correct, you still get 'Access denied'. Conversely, alternating correct and incorrect endpoints doesn't activate Flood even after 10 incorrect attempts (ie// 20 attempts altogether), which is as it should be - it's only 10 incorrect ones in a row in a short space of time that would be suspicious/problematic. All of this is corroborated in the logs. Cheers @the_g_bomb.

How did you get on with your testing @i-trokhanenko? Please let us know and I will look at merging.

Thanks @the_g_bomb and @i-trokhanenko, also reviewed and tested and looks good to me as well. Committing forthwith.

Minor changes also checked, thanks @i-trokhanenko! Merging....

Code reviewed, spun up again on simplytest.me and then phpcs run on for again, with success, thanks @the_g_bomb!

Also checked with simplytest.me, code looks good too, thanks both.

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

@apaderno - many thanks for your comments and assessments, and apologies that it's taken so long to get back to you, but we just wanted to let you know that we are working through the recommendations you made. Hopefully they will all be done soon!

@TheodorosPloumis - closing this issue now due to inactivity, please do get in touch should you want to revisit.

@TheodorosPloumis, many thanks for your useful comments and suggestions - indeed, I have opted for your "better" option and added endpoints to their own page, the latter of which is reachable from the Site Guardian settings page or from the main menu; please see screenshots of the new page and how to get to it. I hope this adequately addresses your needs/concerns, do let us know. Marking as "Needs Review" for your perusal (I also changed the title of the issue to reflect its contents more accurately).

Endpoints page

How to get there

Thanks to you both, do @the_g_bomb 's comments answer your concersn @tijsdeboeck? Moving to 'Needs Review' for you to check.