That's it so far! To review!
The general functionality is implemented, along with a basic test! Anything to add?
Well, it's ready for review again!
Done! Surely, a test of the Form Save behavior for the notes would be necessary, however this test does not exist in this code version but is being implemented in ✨ Add "status" option to allow enabling / disabling snippets Needs work , so it's the same thing as with the unrelated change from before. We just shouldn't forget about this once both issues are merged!
I'll be implementing your proposed changes quickly.
Done with that!
Found some things left over/new:
- A few missed js or Js spellings
- I found out that CSS is also inconsistently spelled sometimes
- A few comments where apparently JS is confused with CSS
I'll be fixing that quickly, no offense, of course! :)
Alright! :)
I guess it would also be easier for us to check for both tags in both cases, as we could then only add this code to the FormBase instead of both forms, and it would not make any notable difference (JavaScript usually doesn't allow <style> at the beginning anyway and vice versa).
That's definitely a good feature, but if we're simply searching for the tag via string search, we might run into an issue:
Let's say that I want to add a string within my JS that contains either <script> or <style>, then the Form would block that, even though this might be necessary in some cases. So would we just be searching via trim() followed by str_starts_with() / str_ends_with(), or do you have a better idea?
Small change, big difference! :)
I'll be starting to work on this issue now!
Postponed on ✨ Move the local endpoint into a submodule Active .
So, back to review again!
I think we need to add a new test for the forms here, to check if everything is saved correctly, so I'll be doing that.
I do believe that's already it! As far as I'm aware, everything works as expected (the base functionality was already there, as mentioned). A tiny but not unimportant change!
The functionality of the CSS and JS config entities having a status value is already there and they can be enabled and disabled via command in the list view. The target now should be to simply add a list column displaying the status and an additional checkbox in the Form where the user would expect it.
That about wraps up everything that is supposed to be done in this issue, I feel like the module is in a great condition now! :)
Please review!
The AJAX integration will be handled in a follow-up issue as that will be another bit of work and as of now this MR is functional so we can merge this right away before continuing with AJAX.
Once everything is done here, and it is RTBC, we can also set all the child issues to fixed so that everyone who worked on there can get their fair share of credit!
LRWebks → changed the visibility of the branch ajax-multi-value to hidden.
As of now we have decided to implement the AJAX calls for additional notification objects in a follow-up issue, so you can check the Form now.
These settings will be properly removed in 🌱 [Meta] 3.x Feature / Concept Improvement issue Active , where we are also fixing some other problems. No need to continue any work in this issue, but we'll still keep it open as a reminder.
I've been thinking a bit about this part of the plan:
Create a new select render element inside "fences_field_formatter_third_party_settings_form()", called "fences_presets", where the preset config entities can be selected from, note that this form element should be purely cosmetic and not save any real value in the config, it's just used to be picked up by the JavaScript.
The question is: Should it be purely cosmetic, though?
I believe that I would be more handy if the selected preset is saved in the config, so that it later appears as the selected option and the person editing the form knows that the values match this preset.
Example: You create about 50 presets (a lot) and then use one of the presets to pre-set the values of the display settings form. Later you re-open the settings page, but since the select value isn't stored anywhere, the select will again say "No preset" and you don't know which one you picked... If the preset select value was saved, you'd know which preset is currently active OR you get to know that a certain preset was used in the first place, and you cannot simply tweak any value because that might ruin the layout.
Also, if you DO tweak settings that were originally filled in by a preset, the selected preset name would still be visible in the select, and you would know which one to pick if you want to revert your small tweaks again. That'd be really handy, in my opinion.
Quick question: In the issue description, you state:
Add a "default_preset" config entity through adding a config entity yml in config/install. Which holds the "Default Preset". This preset should be selected on default, when being on the formatter settings page and having this module installed.
Why would we want to do that, though? The default values apply either way.
If it is just in order to have a preset that appears when we open the config so the preset select isn't empty, wouldn't it be better if we had a "No preset" option that is selected by default and does not change anything?
This would also help with the follow-up issue so that when config entities are changed within the display settings form, we had the option "No preset" so that no preset would be changed if the values are changed within the display settings form. (If you make custom changes and don't want any preset to be changed due to those, you select "No preset" and do your changes then)
Well, let's finally tackle this issue then! I'll start with this now.
LRWebks → made their first commit to this issue’s fork.
No more ideas why no emails are being sent in the test (or are not shown in result). Please help!
Someone else has to resolve the thread though, as I'm not maintainer
Removed the deprecated command and replaced it with an injected Drupal service call. To review it is!
Final improvements, works great! :)
Just tested this with @Anybody! Works as expected, errors are still logged and the error is gone on cache clear.
RTBC!
Now that this issue is done and your review didn't raise any more problems, RTBC?
I can confirm the steps to reproduce from #12
Simply installing this module with Webform produces the error.
@Anybody will take a look soon.
We've just installed 1.0.1 (fresh) on simplytest.me, set the configuration, then upgraded to 2.0.6
Settings page still works as expected, no error or WSOD.
Feel free to review the MR, but we should have clear steps to reproduce this. For that reason, setting this to Postponed.
MR!61 works as expected, thanks!
LRWebks → changed the visibility of the branch 3393158-create-a-photosipe to active.
LRWebks → changed the visibility of the branch 3393158-create-a-photosipe to hidden.
Currently, I am unsure of how to test this, as generating a PHP error manually to receive the email causes PHPUnit to stop the test... Should we "Postpone" / "Needs Review" and outsource the test to 📌 Add basic test coverage Needs work / keep this issue as is?
Currently, I am unsure of how to test this, as generating a PHP error manually to receive the email causes PHPUnit to stop the test... Should we "Postpone" / "Needs Review" and outsource the test to 📌 Add basic test coverage Needs work / keep this issue as is?
All threads are resolved again, back to review!
Created a proper README.md in accordance with the suggested Drupal guidelines for that file. Please review!
Assigning this to me, I'll take care of this.
Yes, that look better than before - nice suggestion! :)
Well, the new functionality has been implemented and is working for me!
Of course this still needs the addition of proper tests, yet I thought better let someone take a look at this first! Perhaps we should outsource the tests to
📌
Add basic test coverage
Needs work
anyway?
A quick suggestion for the default subject and body messages with included tokens (I tried to go after the on-page Drupal error look for better understanding):
PHP [watchdog_mailer:type] at [site:url]
A site reported the following:
[watchdog_mailer:type]: [watchdog_mailer:message] in [watchdog_mailer:function] (line [watchdog_mailer:line] of [watchdog_mailer:file]).
Channel: [watchdog_mailer:channel]
Severity Level: [watchdog_mailer:level]
Caused by user: [watchdog_mailer:user] (IP: [watchdog_mailer:user_ip])
At timestamp: [watchdog_mailer:date_time]
Links:
[site:url]
[watchdog_mailer:link]
Full backtrace:
[watchdog_mailer:backtrace]
Also, here's an example of a filled out version of this, and I think it looks quite clear and understandable:
PHP Error at mysite.com
A site reported the following:
Error: Undefined function myFunc() in myOtherFunc() (line 12 of web/core/modules/custom/mymodule/mymodule.module).
Channel: php
Severity Level: 4
Caused by user: 2367 (IP: 123.456.789.012)
At timestamp: 1234567890
Links:
mysite.com
mysite.com/admin/config/development/logging
Full backtrace:
bla/bla/bla/bla line 1
bla/bla/bla/bla line 2
bla/bla/bla/bla line 3
bla/bla/bla/bla line 4
bla/bla/bla/bla line 5
bla/bla/bla/bla line 6
bla/bla/bla/bla line 7
Everything resolved, back to review!
Works and is ready for review!
I'll be tackling this one!
Implementing these options now!