Simplify Prepopulate implementation with the Entity API

This makes a lot more sense for the following reasons:

• There's less code.
• It does things in the Drupal 8 way.
• It doesn't have to dig down into deep messy array structures.
• It's less error prone.
• It acts on entities directly, instead of forms.
• It'll work better with the dependency injection service container (#2884297: Move the code from prepopulateController into a service and use proper dependency injection).
• It won't require as many complicated workarounds for different field types.
• It's easier to read.

I'll work on a patch to implement this, including what we need to do for #2884297: Move the code from prepopulateController into a service and use proper dependency injection. Afterwards, I'll contact the maintainer about it, and then propose that we get this in first, before looking at any other open issues. It looks like this will solve many of them. For any that still exist, we can re-roll them after we've got this nice new clean base set up.

> It acts on entities directly, instead of forms.

What about forms that are not for entities? Prepopulate is meant to work with any form, at the FormAPI level.

Then this might be a new module: Entity prepopulate? Anyway, hook_entity_prepare_form() is very useful for most of our prepopulate use cases.

I don't think we need two modules for this though. I'll try to come up with something that does it the new way if possible, or falls back to the old-ish way if there's no entity.

I still need to prevent the non-entity case from operating on entities, but here's what I've got so far.

I've tested this with entity forms and it works for me.

As far as non-entity forms go, I couldn't find any evidence that these were ever supported in the Drupal 8 version. The old code simply sets values for form widgets, but these doesn't exist in non-entity forms.

If at some point later someone wants to add support for these, he/she can create a new subclass of Prepopulator that handles this case. That's exactly what I had before, a NonEntityPrepopulator class, which ran the original code. But I ripped it out as it's no longer useful; it only works on entities anyway.

There's an API change here in that we no longer need the edit[], but this isn't actually a problem because we're still in alpha.

Better title.

Another thing occurs to me:

  if(method_exists($entity, 'hasField')) {

That only support fieldable entities, and so doesn't work at all for config entities.

I had to implement something custom based on this patch for my current project, so I wasn't able to do a full review (I had started on a review and run out of time to complete it.

Instead of...

if (!method_exists($this->entity, 'hasField')) {

... I was going to suggest that the code in EntityPrepopulator::prepopulate() calls both $this->entity->hasField() and this->entity->set(), and assumes they act with the parameters / return types defined in \Drupal\Core\Entity\FieldableEntityInterface, so it might be more correct to check...

if (!$this->entity instanceof FieldableEntityInterface) {

Thanks for the feedback everyone. See attached patch. Concerns are now separated between the base class and others. (Well, only one other so far.)

#9: Someone can now create a ConfigEntityPrepopulator class that extends EntityPrepopulator and overrides hasFormField() and setFormField().

#10: Updated the check to do it your way.

Thanks @colan, I tested patch #11 and it solves my issues with taxonomies checkboxes, gives cleaner URIs and looks much more robust. Just took me some time to figure out the new syntax and the edit[] drop, I think USAGE.txt should be updated by this patch too or even included in the UI as Help. And it might be a good idea to open a 8.x-3.x branch for this, so that current users get the word that the URI patterns they are using will have to change after updating the module.

I just wanted to drop in and say that this works for my use case, where I am prepopulating an entity reference to a Group entity. I'm making a distro so I will keep this module in a "patched" folder, and will lbe watching this thread for updates.

I worked form e for entity reference fields.

Now that I've actually looked at code, which is very simple btw, I saw that by doing this, we lose the ability to do further form alterations, like hiding the field altogether. I went in to see if I can add least a prepopulated class but I don't think that's possible with this approach without going back to hooking into all forms.

It appears this removes several changes made in the previous security advisories. For example Prepopulate must act in after_build to ensure no other module has affected whether the current user has permissions to set/alter a particular field. Absent this portion of the cycle some other module could act to remove a users's permissions but the Prepopulate module would violate this by having already set the module.

I would suggest reviewing these issues for some background:

• #2682661: Port SA-CONTRIB-2016-009 to the 8.x branch
• #2679839: Container elements and their children cannot be prepopulated in the 7.x-2.1 security update release
• #1309934: After prepoulating fields (new) content can not be saved if it was edited before

#252053: Allow admins to hide prepopulated fields is an old (very old) issue that has some of the discussion around having configuration items to describe which forms/fields the Prepopulate module is allowed to act upon. From a security and site building perspective this seems to me like the ideal way to handle it. Adding a configuration would also make it simple to allow a site builder to set a custom name for the field as it would appear in the url. Something as simple as a keyed-array:

'a1' => 'node_form::comment_body',
'something' => 'form::field',

... then this site would be allowed to use a1=myComment in the url ... this solves several things, like it's possible to have prepopulate on a site and not set one's site up for comment spamming with prepopulated links, shorter urls, cleaner labels. Additionally it would allow, at the site owner's option, setting administrative fields, hidden fields etc, which cannot be done in a secure, generic manner.

Thanks for that background! Given the above, I'd like to propose the following plan. Feedback is welcome.

1. Create a new 8.x-3.x-dev branch.
2. Commit #11 to it.
3. Create a release blocker issue for whitelisting forms.
4. Create a release blocker issue for whitelisting fields.
5. Create a release blocker issue for updating the documentation as per #12.
6. Move #252053: Allow admins to hide prepopulated fields to the new branch for work on it there, but don't make it a release blocker.
7. Create a new issue for field aliasing in the new branch, but don't make it a release blocker.

Thoughts?

#17 Makes sense, this would help the project along.
Now I'm seeing all kinds non drupal 8 usages. I could start patching everything, but when above would be implementent it gives me a good base to work from there. Also this would prevent merge conflicts and having to rewrite those patches again.
So I would say commit #11 and go from there. I'm totally willing to dedicate some time to the the remaining issues, but it has to be in de comming week.

I could patch the current version with #11 and work from there if we decide this is the way to go.
Any thoughts?

Looked deeper into #11, I see type hint string is used, this is supported from php 7.0 and up.
Officially drupal 8 core currently supports 5.5.9+ But it will PHP 5.6 and 7.0 will reach their end-of-life at the end of 2018.
Also php 7.1 is the currently recommanded version.
So I'm agreeing to let the type hint string in.

The dependency injection looks good, much cleaner code this way! :)

+1 for #11

I guess this needs some more work.
When I try to fill a multiple field:
title=test&field_selectors[]=foo&field_selectors[]=bar I get the error:

The website encountered an unexpected error. Please try again later.
TypeError: Argument 2 passed to Drupal\prepopulate\Prepopulators\EntityPrepopulator::setFormField() must be of the type string, array given, called in /var/www/monitor/htdocs/modules/contrib/prepopulate/src/Prepopulators/Prepopulator.php on line 35 in Drupal\prepopulate\Prepopulators\EntityPrepopulator->setFormField() (line 65 of modules/contrib/prepopulate/src/Prepopulators/EntityPrepopulator.php).

I'm looking into this.

I changed the accepted parameters for value to mixed, this allows an array to be used as value.

Now I can successfully call node/add/article?title=tes&field_selectors[]=foo&field_selectors[]=bar&field_selectors[]=foobar&field_selectors[]=barfoo

I just tested this with key_value_field and this also works:
title=test&field_selectors[0][key]=asd&field_selectors[0][value]=test&field_selectors[2][key]=test2

Thanks for working on this!

Here's a conversation I had a while ago with the maintainer, his thoughts and then my response. I can't think of a reason why it shouldn't be made public (as it's not revealing anything new), and provides a bit more insight. He's been unresponsive lately so not sure if he's still around, or even interested in this module anymore. I suggested initially that we should start a new branch for this.

On 2018-01-03 05:36 PM, jbrauer wrote:

Given the module’s history and security advisories I’m not particularly comfortable opening a branch with known insecurities. I think with a few changes, specifically moving back to after_build and limiting the fields that can be prepopulated we could maintain the security, even for those who adopt the nightly without understanding that it might not be secure. From there we’d have a building point that maintains the current state of security and provides a way forward. In that way at the same time a replacement goes in for the current protections they can be removed at the same time.

I understand where you're coming from as I'm also a fan security-minded thinking. The other side of the coin though, from a development perspective, is that it's challenging to handle multiple issues in a single patch. This makes it rather unwieldy, making it difficult for us develop a huge patch keeping everything in mind, and it also is harder to review smaller patches. Dividing and conquering would be my favoured approach here.

How about a compromise?

What if we:

• Create a 3.x branch, but don't recommend it (keep 2.x recommended).
• On the project page (and possibly elsewhere), state the following:
  

  For Drupal 8, 8.x-2.x-dev is the recommended branch at this time. While there is an 8.x-3.x-dev branch taking advantage of new Drupal 8 features, it is an unfinished rewrite under heavy development with known security vulnerabilities. Do not use it on Production sites! If you're interested in this initiative, please help us with the remaining critical issues at [link to view of 3.x release blockers / critical issues]. With your help, this branch can be made stable as soon as possible. For background information, see [link to this issue].

This has the added bonus of being a call to action, which will certainly speed up development.

I recognize that the stated policy of the Security Team allows us to have a knowingly insecure -dev branch but I’d really like to avoid that, not just for Prepopulate but also for the bad press it can give the overall project if others can point to cases where necessary security is removed even if it’s with the good intent to have it added back later.

When I look at this initiative, I don't see security being removed, I see a completely new in-progress implementation that has yet to be finished. We're rebuilding from scratch, adding the necessary components as we go, but making it clear that the project isn't usable until all of the critical elements are in place. Think of building a house. You can't build it all at once in a single shot. First you build the foundation, then add the walls, and finally the roof. As long as the future inhabitants are aware that they can't live in it until the roof is up, everybody's happy.

What do you think?

...and then I never heard back. So I think we have some options:

1. Somehow get in touch with the maintainer and convince him to allow the new branch.
2. Take ownership of the module if he's no longer responsive via Dealing with unsupported (abandoned) projects.
3. Fork the module.

These are in my order of preference; I'd rather not fork the module.

@scuba_fly: I think the best way to start this process is for you to open an offer to co-maintain the module as per the second option above (or take over the existing #1138466: Offering to help maintain prepopulate module), as it'll lead to either of the first two options if possible.

Thank you for your reply and suggestions.

I Agree having a seperate branch, or even 3.0 version will be the best way to go. Since the module only has an alpha release I would not go for a public recommended branch untill security tested. On the other hand we want it to be public so people can contribute.

I have send jbrauer a message.

I'm seeing my patch was not complete. The interdiff is corect, but the patch file is missing stuff. I'll upload a new patch in a moment.

And here is the new patch.

Patch #26 does not apply somehow. See the interdiff for what I want to change.

I am not comfortable making a public branch (even one not labeled as recommended) that not only has known security issues, but specifically reverts the security advisories previously published for this module. The patch considered here should be relatively easy to alter to maintain the critical security of protecting fields and ensuring the user has appropriate permissions. Once these concerns are addressed in the patch a new branch seems like an appropriate route to go.

Thanks for your comment, I was on vacation and busy the last coupple of months.

I think your thoughts are clear. It is only hard to do all this work in one big patch file. My suggestion is to create a fork of the project, don't list in on d.o en work on it with a few people. Once converted to the new Entity API, create a patch / new branch.

Another aproach could be to start with write tests for the security issues on a different branch. ( So there would me no workable code there ).
Then start working on it untill the tests pass. And then make it public.

I think we also want to address #28. So between that and this small point, punting back to NW.

+++ b/prepopulate.module
@@ -3,38 +3,17 @@
+  EntityPrepopulator::create(\Drupal::getContainer(), $entity)->prepopulate();

+++ b/src/Prepopulators/EntityPrepopulator.php
@@ -0,0 +1,69 @@
+  public static function create(ContainerInterface $container, EntityInterface $entity) {
+    return new static(
+      $container->get('request_stack'),
+      $entity

Why can't this be a service and inject all the needed things from there?

Making it a service is a good idea; it just wasn't on my radar when I originally worked on this stuff, and hadn't done it yet. Now that I've written a bunch, I agree it makes sense to do it here (unless I'm forgetting something).

So, this improves some things and switches over to using a service. It will not pre-populate if the field already contains values. It also will only prepopulate text type input fields. Those radio and checkboxes fields on the user and permission pages are safe.

#33: Would you kindly provide an interdiff for easier review? Thanks.

#33 was an entire reboot. No interdiff for that reason. Should have mentioned that.

Okay, great! So this should fix #16 / #28 then.

It addresses many of the largest security issues. It now uses after_build and has a whitelist of input types. Of which only text-ish fields are supported. It doesn't allow overwriting an existing field if that field is already populated.

What it doesn't do is provide a field mapping and explicit opt-in list of fields that can be pre-populated. I'm not sure if that is needed to satisfy security findings or if that is a nice feature that we can add later.

I'd personally like review from a security perspective to see if there are any holes in that latest patch.

I like that you added hook_prepopulate_whitelist_alter(), that opens the door for other modules to support it. It'll need some documentation, but thumbs up.

API docs added. Unless I hear to the contrary, I'm probably going to commit this patch in the next few days.

Couple more tweaks.

Still working on it, or already fixed?... I see it was commited, but issue still "Needs work" :)

maybe this is related.