🇬🇧United Kingdom @AdamPS

A new issue sounds good to me thanks

I have added credit now. Does that have any effect or is it too late? If too late, is there anything else I can do?

I do appreciate your contribution and I'm sorry if didn't make the correct actions in the system.

Sorry I hadn't realised that it wasn't added automatically.

In Drupal Core, I can't find any entity classes that have a toString function, so this doesn't seem like its the right solution. I guess you probably have the wrong URL pattern

OK here's as idea. I feel that it would be worth creating a new major release if we did a more widespread update of the entire module to use PHP8. We could adopt constructor promotion, add types to class variable and function returns, and anything else relevant. It's not a big module so it probably wouldn't take very long.

Otherwise we could postpone this issue for now until there is another reason to create a major release.

Before we make a solution, we need to understand why the recursive rendering in paragraphs library and webform is OK. Why is it not creating an infinite regression, when our regression prevention is detecting the start of a regression?

Another scenario is sending a simplenews newsletter. The same entity is rendered repeatedly for each recipient, possibly hundreds of times in a single batch. If the recipient are Drupal users, then the entity is rendered each time in that specific users context, meaning that it cannot be cached. Clearly in this case there is no recursion.

Great many thanks

Thanks. I'll commit this anyway. I believe #3453055 might not be needed, however if I'm wrong we can easily commit it after.

In theory we ought to create a major release for a non-BC change, however it seems a bit excessive in this case.

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

Thanks.

It's strange - the report says "and an error will be thrown from drupal:10.0.0." however I just tested on D10 and there was no error. Also everywhere I checked in Core doesn't have the call to accessCheck.

After much head-scratching I believe that the error report is wrong. The link change report states (not obviously, but it's there): "This change doesn't apply to config entities." Which makes sense because they have no access checking. Perhaps you are running an old version of phpstan, or it's a phpstan bug?

Thanks for the report. I don't use Layout Builder so I haven't seen this myself.

The code that sets #theme to submitted is now in manage_display_entity_view_alter. Perhaps Layout builder does something that prevents this function from being called?

Relying on entity queries to check access by default is deprecated in drupal:9.2.0 and an error will be thrown from drupal:10.0.0.

So this first one is presumably a bug.

The other 3 are all insisting on DI. Personally I don't really agree when it adds loads of code and anyway does anyone care😃? It seems like we are responding to the automatic warning rather than changing it because someone actually wants it. Also the constructor change would break any code that extends the class.

The DI adds a lot of lines, but once D9 support is dropped and PHP 8 is required,

D9 is already EOL and no longer supported and it could safely be removed from the info file.

Thanks

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

Thanks

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

I believe the steps to reproduce are wrong. If you want no recipients then you don't add the adjuster. If you add the adjuster but don't set any address, then you get an error like "You must set at least one To address." On your site where you have the problem, then if you tried to edit the policy then you would also see the same error.

The address adjuster should never have config without any addresses, and it enforces this. If you edit the config programmatically to create illegal config then that is not the fault of this module.

Skip sending is entirely different - it blocks the email entirely which is not the same as having no "To" address (in which case you could still have a "Bcc").

In Node::baseFieldDefinitions() the title field is set to be displayed. Therefore, without this module, when a content type or view mode is added, the title is displayed.

The current code has the same behaviour with this module. I believe the patch would break that.

Thanks

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

Thanks @simonbaese I added a link to the project page

Great thanks I updated the project page

Great nice work - please can you make some quick changes for comments I added in the MR then I will commit

Thanks @elek for the explanation

Great thanks. I committed it however the tests are failing. Most likely it's because they were running with D9.5 before and now with D10.

Probably this line in RrssbTest.php line 54

$this->submitForm($edit, 'Save content type');

needs to change to this

$this->submitForm($edit, 'Save');

or something similar

I believe it is a matter of timing.

BodyEmailAdjuster->build() runs with weight 400. It saves the variables into #context.

Your hook likely runs with default weight of 500. If you change your weight to 300 it should work.

Yes I see your point.

My concern is that the error messages could reveal internal site configuration details that the site owner would prefer to keep concealed. I guess another option is to create a new permission for "view detailed error logs"

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

Sure, however I was comparing this module to Core.

In Core it depends on "Display author and date information".

With this module, you add the date/author fields using "Manage Display" settings. The setting "Display author and date information" is hidden and has no effect.

At least that's how it works for me, and how it's supposed to work.

Yes it's good, very clever thanks.

Currently there are no more NR, and 2 are RTBC ready to merge (including this one). Please let me know when you are ready for me to create a new release.

The user "title" field is called "name" and it is shown

Thanks. Setting status back to Active as there is currently nothing to review.

Thanks

With this module it works differently - you just add the date/author fields using "Manage Display" settings.

I'm open to the idea of a patch that changes from simplenews_node_presave to simplenews_node_update. It feel like a much cleaner approach than the original patch. Beware though, often things in the simplenews code were done for a reason (after hitting a problem) without leaving a comment to explain why, and it can be quite subtle.

I wouldn't like to delete the protection of // Save except if already saving. because any site could have custom code that calls addIssue() and relies on this.

Or we could add a setting on from adjuster "also use as sender" (causes error if there is more than one from)?

I propose we should have a description that explains:
- by default the sender address comes from "Basic site settings" and normally you wouldn't change it
- ensure this server is authorised to send from the specified address to help prevent emails being flagged as spam

Thanks, looks good to me. Can anyone else test it please and set RTBC?

Sorry for the delay

Here is an updated patch

1. name the tag administerusersbyrole_access tag and check all the permissions (view and edit) as described in the IS.
2. use a function for the common code rather than duplicating it

Please can someone review/test?

I understand your situation and I can see that this would solve it.

However I feel that simplenews is completely correct. The problem is that scheduler module is allowing the node to have the published flag set briefly when in fact it the node isn't published. So this patch is adding code in simplenews module (also quite popular with 20k D8 installs) to workaround an issue in scheduler. The code relies on a detailed understanding of the workings of scheduler. It would become particularly messy if the required workaround would vary depending on the major version of scheduler (which would be allowed by Drupal compatibility rules).

I have an idea. It seems like it could solve the bug if the presave() functions ran in the opposite order. The current order is presumably random chance based on the alphabetic order of module names.

So scheduler module could alter the implements order to ensure its own presave runs first, or even change its module weight - this would ensure that the temporary incorrect published status was not seen by other modules.

What do you think?

Great thanks. URLs inside translation should be a parameter, like this line from simplenews_help():

      $help .= "<p>" . t('For more information, see the online handbook entry for <a href=":handbook">Simplenews</a>.', [':handbook' => 'http://drupal.org/node/197057']) . "</p>\n";

The term "sub-admin" is widely in this module, including on the project page and in the readme. The advantage is that it's a single word term to refer to a key idea - whereas "users with related Administer Users by Role permissions" is 8 words.

How about if we add a definition of "sub-admin" once at the top of this help page?

@AstonVictor if this looks good to you then you are welcome to commit.

For many cases we need to add classes to the h1-h5 elements and with html_tag it's easily possible.

Please can you explain how you were adding classes before and why it no longer works?

This module aims to copy Core as closely as possible. The changes in Version 3 improved this and we don't want to go back. The long-term aim is that it could be added to Core. I would expect that whatever you can do with StringFormatter you can also do with TitleFormatter.

Could you use a module like https://www.drupal.org/project/field_formatter_class → (or copy the code into your own hooks)?

The idea in the MR is interesting, however I really doubt that Core would accept it.

No response from OP so please go ahead.

With this module, "Display author and date information" should not be needed and it should have no effect whatever the setting. For this reason we hide the setting to avoid confusion. So I would say this issue is "works as designed".

When enabling this module, I believe this works differently if you have already selected to hide the "Display author and date information".

Please can you explain. In what way does it work differently?

I don't see the same problem

I have very little time now for Drupal, so I will try to leave you as much freedom as possible. Either this or the first one are fine whatever you prefer.

I have very little time now for Drupal, so I will try to leave you as much freedom as possible. You can commit it is you like.

Thanks it's good. Please can you make a merge request? Patches are no longer used for this module and it will soon be the same for all modules. Sorry I didn't spot that the first time😃.

The release note was deliberately vague because it's a security fix so we avoid making it too obvious how to exploit it. The previous behaviour was a bug, it should never have been like that. I understand that this caused inconvenience for you however it was pure chance that your site previously worked. By default unpublished nodes aren't accessible and this module (now correctly) doesn't change that.

AdamPS → changed the visibility of the branch 3436717-private-and-unpublished to hidden.

I doubt that we should do this. This module has been stable for several years. We decided to grant access to those 3 specific fields, but not to others (e.g. status or init). If we change it now then it would affect all sites, and some of them might not want it. If any site wants to change the default then they can use the hook just as you wrote it.

✨ Allow to customize Sender Active seems like a better UX to me. What do you think?

Thanks

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

This need steps to reproduce to allow anyone to help

It looks like a duplicate of 💬 LoaderError Template field--node--uid.html.twig Active

> the Rules module requires the Mime Mail module

It says "a contributed module such as Mime Mail". However it should also work with this module

I can't think of any reason for this. Using a complex mail structure should make no difference to this module - it is the Drupal rendering system that generates the content and this module just inserts it into the email template using the Twig template.

Is it possible that the intermediate mail servers or client are filtering out images? You could log the email as it leaves your Drupal server and check the result.

As developers we don't like intermittent bugs😃. Is it possible that the images are missing according to a specific condition - such as the contents of the cart, or who is logged in? Or can you place an identical order time and time again, with random failures?

For me, this solution is a clearer UX than 📌 Webform from address not set as sender address when using symfony_mailer Needs review .

1) If do this we need to provide a clear warning in the UI, for example in a description text. Firstly normally don't use this, set the site address instead. Secondly make sure to use a valid domain so as to avoid getting classed as spam - we could copy the wording from core.

2) Message.php will anyway add a sender based on From. So all we need to do is set the sender to blank. This ensures that it copies the final from - rather than the current value which might later be altered by a hook.

3) For a new feature, please add a test.

There is a variable you can use {{ site_name }}

Tokens aren't enabled by default for two reasons

• by default there would be no tokens available except a few global ones
• replacing tokens could cause some unwanted replacements of text that happened to look like a token

Thanks that makes sense. Let's also change the label something like "Email address or special code"

This was done like this deliberately.

The idea is that the variables should be replaced in the "normal" template. The wrap template is aimed to be generic, not related to the contents of the email.

Please can you explain why you can't use it that way?

Needs more info to explain how the error is related to this module

This module already has support for cc addresses, please see CcEmailAdjuster

Great thanks

Great many thanks everyone

greggles → credited AdamPS → .

> my client has lots of pages that display a subscribe form,

Agree and this is point 1) from the IS

> Sure, the hook is an option and then tracking that data somewhere, but that means duplicating the data and not having a history for it.

I'm open to adding an extra field with "changes". I guess it could be one field for subscribes and another for unsubscribes??

> but I'm not entirely convinced that the data really should be kept if you *delete* a subscriber

I agree for anon subscribers. They are not normally deleted, instead the subscriptions get removed.

However currently if you delete a user, then the subscriber is automatically deleted. I guess we don't want to delete the history in this case, as we lose proof of consent. Maybe we could change the code to keep the subscriber and remove the subscriptions although I have some doubts.

> My guess is that developers will mostly fail to do so😃.
What I mean is this. Imagine that we add subscriber ID to the history and it can be blank. Developers writing code that reads the history will often not check.

AdamPS → created an issue.

Please can someone review/confirm that this fixes the problem?

> Manually attaching patches is being phased out, so perhaps you can create a patch and Merge Request with the Gitlab integration.

Yes but it's not currently working unfortunately see 📌 Use gitlab-ci Needs work

The original issue was 📌 Track history of subscribe/unsubscribe and proof of consent Fixed . I didn't find anyone to discuss it with at the time so I did my best😃

> used to be a descriptive action like subscribe or bulk subscribe?
actually it used almost always to be "website" 😃

> It has a source, but that's just a route it seems
Why "only" a route. A route seems like an excellent way to pinpoint the source. It's an existing concept with a unique ID and a localised human-readable name. The unique ID doesn't currently have an accessor function but you can write $this->get('source')->value;

Aha I understand now - you wish to know about the changes per newsletter subscription. The current implementation is very similar to an entity revision. True, it doesn't directly store differences, however they can be computed easily - just search backwards for the previous history entity with the same email address and compare the subscriptions field.

I wasn't attracted to the idea of a "changes" field because it's just duplication of information and it can't be generalised to other fields such as email address and in future we could make the history fieldable to match the subscriber fields.

There's also hook_simplenews_subscribe().

> I'm not sure why history isn't directly referencing a subscriber through an ID

The subscription history is independent of the Subscriber object, so it remains after that object is deleted. This could even be used to support "right to be deleted", we could use an index based on a hash of the email value to prevent re-subscription without have to store the original email address.

Possibly we could add the subscriber ID, however code would need to allow for it not being set. My guess is that developers will mostly fail to do so😃.

Thanks @Berdir let's try it

AdamPS → changed the visibility of the branch newsletter to hidden.

AdamPS → changed the visibility of the branch 3421481-you-are-already to hidden.

The requirement is that you must set either newsletters or default_newsletters. Either one can be blank, but not both.

Postponed until June 2024 when support for D10.1 ends - see #4

The only thing that I can guess might be causing the issue is the fact that we use the Drupal LDAP modules to create the Drupal user when they first login.

Aha then I believe this is the cause. The setting only works on the user registration page, please see the description for the 'Subscribe new account' setting:

'None: This newsletter is not listed on the user registration page.
Default on: This newsletter is listed on the user registion page and is selected by default.
Default off: This newsletter is listed on the user registion page and is not selected by default.
Silent: A new user is automatically subscribed to this newsletter. The newsletter is not listed on the user registration page.'

Yes I agree - however we only alter the default when the field is hidden

I see it like this. This issue is about when emails for the contact module are overridden, which is optional.

1. If sites would like to keep everything the same as before then they shouldn't override.
2. If they override, then some parts of the contact_form entity will no longer be used as they are replaced by Mailer Policy. This is intentional and is one of the goals of this module. Maybe even one day Core will take this change.

AFAICS the contact_form entity isn't used outside of contact module, except in test code.

I still feel that the solution in the IS is correct. It's not complicated😃. Anyway this issue maybe isn't the top priority.

I've made two edits that I would appreciate comments/feedback on from the community:

1) I propose that ASAP we should remove the existing warning. It has likely wasted 1000s of hours of site builder and developer time trying to follow the instructions, which a very low likelihood of actually providing anyone with any protection at all. It's so complex to do correctly and so lacking in documentation that I doubt even 1% of those who try do it succeed; they might even introduce security bugs by doing it incorrectly.

2) I have expanded the proposed resolution to automatically take care of as many aspects of the solution as possible to give site builders the best possible chance of getting it working.

Thanks. The comments I mean are these. They give a rough indication of the template without too much detail and describes the default value. What do you think?

* Formats a user reference as a sentence including the date, with an optional
* user picture. The sentence is determined by the 'submitted' template, with a
* default of "Submitted by on ".

AND

* Formats a comment reference as a sentence including the subject and author.
* The sentence is determined by the 'in-reply-to' template, with a
* default of "In reply to by ".

> I don't think it is related to the layout builder module.
Sure then please update the IS😃. I don't see the problem on any of my sites without layout builder so please provide a sequence of steps to reproduce that should work for me too.

> It should be configured via theme suggestions.
Please assume a standard install of Drupal Core and this module: the only hook_theme_suggestions() implementations are from that.

The question in #12/#15/#17 is essential for me as part of the review process. I don't know what else I can do to explain it😃. We need to understand if this patch would change the behaviour of this module, altering which templates are used for existing sites.

On the page I linked to in #4 is says this:

When prompted for Minimum Stability []:, hit enter to use the the default of "stable" stability. This will have no effect on websites where your code is installed, but may affect which versions of dependencies are used when running your project's tests. Stable dependencies during testing is preferred to unstable ones.

I searched all the modules installed on one of my sites. The majority leave it out altogether so perhaps that's best??

Great thanks I deleted the old tests