Pipeline fixed!
Okay, perhaps the pipeline failure isn't that unrelated, I will take a look.
The base functionality and test coverage is there now, PHPUnit seems to fail due to an unrelated test from a different commit.
The machine name field already works on TokenCustomTypeForm.php, but not on TokenCustomForm.php. @anybody suggested to me that he will take another look at this. For now, back to “Needs work”.
The install file is cleared of the mentioned patterns now and the update hook removes them without traces from any existing installation (update hook manually tested).
After looking through the default reject patterns, the ones that I would personally remove because they would be used often in a regular comment or article:
- http://
- https://
- www
- href=
- great website
- keyword
- :
- look at the
lrwebks → created an issue. See original summary → .
Everything should be working now. I have also tested the update hook manually and the conversion from the comma separated list to the line separated list works absolutely fine.
The culprit of the problem is the following, as far as I can see:
In line 280 of protected_forms.module there is a helper function to turn the comma separated string from the settings page into an array. Sensibly enough, it also strips away the whitespace from all array items, since some users put a whitespace after each comma and others don't:
// Trim white spaces of array values in php.
$array = array_map('trim', $array);
This of course also deletes the placed whitespace in front of the colon, that is then flagged as spam.
A simple but not very gentle solution would of course be to not trim white space and force the user to not put spaces after their comma…
What do you think about this regarding a solution?
I have definitely run into this issue before, where not enclosing strings in YAML was leading to unexpected behavior, so I will certainly take a look. I am also fairly confident that the colon is part of the Latin language character set, since it is a part of ASCII, which is almost entirely contained within the Latin set. I'll check it out!
Everything should be running now!
Regarding #9, I have re-added the appropriate previous implementations as fallback behaviors on all occurrences, so this should be done now!
Now that the tests are in place and all pipeline jobs are fixed, I think we're done here for the moment!
This has to be merged before I can copy the README over to the module page, so please review.
Here are some more focused and demonstrative screenshots. @Anybody, what do you think?
Hopefully everything should work now! The problem was related to the fact that the setting was named destination, the name was already taken somewhere in the parent hierarchy.
lrwebks → changed the visibility of the branch 3482631-validate-redirect-path to hidden.
Case dismissed as we have found the reason that is actually not related to this at all. Thanks anyway!
We're having another strange behavior around the path element using '#convert_path' => PathElement::CONVERT_NONE, the value of $element['#value'] is always the #default value and not the actual value when submitting the form. I also checked this within public static function validateMatchedPath(&$element, FormStateInterface $form_state, &$complete_form) in PathElement.php
Can someone confirm this and is this related? Thanks!
Success! Found a helpful snippet in the help hook of the Block module:
Url::fromRoute('help.page', ['name' => 'project_wiki'])->toString();
And that actually works. Apparently the routes from the help module are generated dynamically like: help.page.{name} and if we return the name of our module as parameter, it results in the route that we want!
I (at least) am happy to have learned something new about the Drupal structure today! To review!
It appears that the path form element is only ever used in the ContactFormEditForm.php that you also linked in the issue summary, where it is then additionally validated in the validate function via the `pathValidator`, (makes no sense to me, this works in our module without it being there entirely) and then submitted. But since I know first-hand that the approach with the `pathValidator` also has this weird behavior, I think that this would not be the right solution for us...
After a bit of testing, it seems that both approaches work identically in practice. Considering that path already is a valid form element, I would definitely use this one as it's existence to me proves that this approach is the Drupal way in contrast to #9.
What I noticed however in both implementations: When entering an absolute URL like https://www.example.com or similar to that, instead of giving a warning, the page says that the config was saved, but the input is replaced by the previous valid path, e.g., `/admin` that was saved before. That feels like unexpected / unwanted behavior to me. What do you think?
I'll implement the necessary tests into MR!13 (the one with 'type' => 'path' now.
Added the CR! Now we are left with the Usability Signoff.
Changes to the tests as well as new tests done, however phpunit does not seem to be overly happy about the schema for the conditions.
I see two options here:
1. Define the schema differently (I am not that familiar with the dynamic schema for conditions though)
2. Use the actual settings page to change the settings in the tests (Not a friend of that since it should also work when changed via configFactory)
Any suggestions on how to proceed?
Patch works, RTBC!
Done, see the Drupal Localization page for review.
99% of the work here is done, still having a few troubles with the waitForElementVisible function not behaving as I would expect it to.
Done.
README.md is already done in 📌 Initial implementation Active , I'll sort out the module page now!
That's it for the initial implementation as far as I'm concerned!
Everything's done now, including a quick overhaul of the module page and READMEs. @Grevil for the review? Don't forget to also review my changes on the module page!
If this is a relative URL NOT starting with / […] leave untouched
I think this case should also be prefixed with the module path, as this would still give access to more folders than it should (security). /img.png could easily be something like /my-secret-module/secrets.png, which the markdown should not have access to.
In my opinion the best approach to this would be to only allow relative URLs in one of these forms as these can all safely be redirected to the path within the module:
image.png or folder/image.webp/image.jpg or /folder/image.svg./path.jpeg or ./folder/image.jfifSo if I understand this correctly, you want the nesting to happen with the entries and their respective categories (one layer of nesting), right? Or do you want items to be able to contain other sub-items now (infinite nesting)? This would also require a change of how entity contents are created, as well as a new markdown metadata key.
Okay, so now the list page does not show any entries anymore on my end!? It was just working a few minutes ago... Tugboat also shows no entries. What is this now?
Yes, I can quickly implement one now, no problem!
Done!
Now that much has changed with the module and the routing, I think it is best if we postpone this until the very end, where we can take a final look at the routing in here. If this is needed sooner, we could always move this up in the list again.
This does it, the admin theme works now. I think it is best not to meddle too much with the routes here, as we are planning to do a big routing overhaul at the end, see 📌 Refactor URLs and Names of Project Wiki related pages Active for that.
This improved controller build logic should do the trick now! It works for me at least. The new foreach logic should also resolve a problem that we didn't even see yet, since we haven't had multiple Plugins installed before. Only the first one would have been shown in that case. :P
lrwebks → made their first commit to this issue’s fork.
Indeed, I have looked through the generated HTML files and tested it many times locally as well as manually (It definitely works with manual testing in the UI!). My impression is that the failure is related to the test environment itself and the fact that the results don't get loaded/unloaded immediately, but after a small delay due to JS (the test check might simply be too fast for listjs to load).
@Grevil asked me to let him take over to work on the failing test, so I'll assign this back to him.