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.
Yes, the test is unrelated as is currently being tackled by @grevil and me in 🐛 Fix coding standard issues Active .
Here are the screenshots, both with and without wiki entries on the list page:
Well, I first assumed that the test was failing because of load times (leading to inconsistent checks), but after letting the test sit for a whole 5 seconds after every search query, the error persists, and it is becoming incomprehensible to me. :P @grevil wanted to take over, so I will let him take a look at this.
As far as I can see, a lot of code has changed since we have opened this issue (nearly every file has a merge conflict). So I suppose it would be faster for us all if I would just open a second MR with the current 1.x branch and quickly resolve everything there.
Promoting this to major as a lot of issues simply have unrelated failing pipelines due to this, so I will try to get this one out of the way quickly.
Since this will be fixed in 📌 Place "Create new entry" in the typical location (top right) Active as mentioned in the issue summary and the implementation in the MR of that issue is already working, I think we can close this one.
I'm raising the priority as this is a blocker for 📌 Show "Create new entry" also if the list is empty Active and I will work on this first.
Here is a screenshot of the new ban admin UI:
And here is the "Delete Multiple" confirmation form:
For easy viewing I have also added the screenshots to the issue summary.
Hier ein Screenshot des aktuellen Standes:
Well, the failing test was green locally, so I decided to try to run another pipeline, and: No more fails! Seems weird to me though, as I did not update the fork or anything similar in between pipelines, so this feels like a weird PHPUnit hiccup… It could be related to the fact that the failed FunctionalJavascript test seems to use the waitForElementVisible() function a lot, which can fail randomly if the page loading takes too long.
Anyway, back to review!
@anybody regarding #9
- I'm looking into it right now
- The function
pathauto_pattern_validate()from thepathauto.modulefile is called from the form via the following line inPatternEditForm.php:
'#element_validate' => ['token_element_validate', 'pathauto_pattern_validate'],The other checks for missing tokens and whitespace at the end of the path were already placed in the
pathauto_pattern_validate()function, which is why I placed the new check there as well. I suppose that we could try to move this whole functionality over to the form if it is not used by any other part of the module, but the current implementation works fine right now. - Also, are you able to resolve the two threads that I opened? Resolving your own threads seems to be disabled in the GitLab UI for this module.
I've closed all issues that were left for 6.x as outdated now. Only 21 open issues left, while there were over 350 of the old ones. :P
Drupal 6 is EOL and no longer supported. Closing this as outdated for that reason. Thanks for your contribution!
Drupal 6 is EOL and no longer supported. Closing this as outdated for that reason. Thanks for your contribution!
Drupal 6 is EOL and no longer supported. Closing this as outdated for that reason. Thanks for your contribution!
Drupal 6 is EOL and no longer supported. Closing this as outdated for that reason. Thanks for your contribution!
Drupal 6 is EOL and no longer supported. Closing this as outdated for that reason. Thanks for your contribution!
Drupal 6 is EOL and no longer supported. Closing this as outdated for that reason. Thanks for your contribution!
Drupal 6 is EOL and no longer supported. Closing this as outdated for that reason. Thanks for your contribution!
Drupal 6 is EOL and no longer supported. Closing this as outdated for that reason. Thanks for your contribution!
Drupal 6 is EOL and no longer supported. Closing this as outdated for that reason. Thanks for your contribution!
Drupal 6 is EOL and no longer supported. Closing this as outdated for that reason. Thanks for your contribution!
Drupal 6 is EOL and no longer supported. Closing this as outdated for that reason. Thanks for your contribution!