Opened 17 years ago
Last modified 6 days ago
#12134 assigned Bug
contrib.admin.RelatedFieldWidgetWrapper.__deepcopy__() should copy() the widget attrs
| Reported by: | James Bennett | Owned by: | Garvit Sharma |
|---|---|---|---|
| Component: | contrib.admin | Version: | 1.1 |
| Severity: | Normal | Keywords: | |
| Cc: | Triage Stage: | Accepted | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
Otherwise it ends up with a shallow copy which reuses the same attrs dict across separate widget instances, playing merry hell with form classes which want to change widget attrs on a per-(form)-instance basis.
Attachments (1)
Change History (12)
by , 17 years ago
| Attachment: | 12134.diff added |
|---|
comment:1 by , 17 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:2 by , 17 years ago
comment:3 by , 16 years ago
| Severity: | → Normal |
|---|---|
| Type: | → Bug |
comment:6 by , 11 years ago
I think #26213 gives a demonstration of a behavior which this ticket may fix.
comment:7 by , 5 weeks ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
Claiming — patch ready, PR to follow.
comment:9 by , 6 days ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
Yes, I’ve addressed the requested changes. I’ve now changed the triage state to “Ready for checkin” and set “Needs improvement” to false.
follow-up: 11 comment:10 by , 6 days ago
| Triage Stage: | Ready for checkin → Accepted |
|---|
Hi Garvit, please refer to the Triage Workflow. RFC is a state marked by a reviewer, not the PR author. Thanks!
comment:11 by , 6 days ago
Replying to Clifford Gama:
Hi Garvit, please refer to the Triage Workflow. RFC is a state marked by a reviewer, not the PR author. Thanks!
Thanks Clifford, understood. I'll leave the triage stage for a reviewer.
I think we need some more analysis of why this patch is correct/needed. Here are some simplified extracts of current code:
This means:
That all seems to be correct. The attached patch would actually break the second assert, which I'm pretty sure will break the ability of the class instance to work as a wrapper.