Problem/Motivation
Working on #2403229-8: language.negotiation configuration can have overrides bleed in I found that language_configurable_language_insert()
& language_configurable_language_delete()
could be moved to \Drupal\language\Entity\ConfigurableLanguage::postSave
and postDelete()
methods
Proposed resolution
Move them to entity because no reason to split the logic within the same module
Remaining tasks
Commit.
User interface changes
None.
API changes
No, just removing 2 hook implementations.
Beta phase evaluation
Issue category | Task because the code works as-is, but the code organization makes it hard to follow. |
---|---|
Issue priority | Normal because the code works the same way either way. |
Disruption | No disruption. Hook implementations are not part of the API, they should not be invoked as standalone functions. |
Comment | File | Size | Author |
---|---|---|---|
#60 | interdiff-2462729-58-60.txt | 921 bytes | yogeshmpawar |
#60 | 2462729-60.patch | 3.33 KB | yogeshmpawar |
#51 | 2462729-51.patch | 3.49 KB | andypost |
#51 | 2462729-interdiff-50.txt | 615 bytes | andypost |
#50 | interdiff.txt | 615 bytes | andypost |
Comments
Comment #1
Gábor HojtsyLooks like these were not converted when postSave() and postDelete() methods were introduced.
Comment #2
rpayanmPlease review.
Comment #4
sidharthapHere is a updated patch. Not sure if it is the right way.
Comment #5
rpayanm$this is not accessible in static context.
Let me try.
Comment #10
andypostProbably this affected by parent issue but anyway
Insert only!
if ($this->isLocked() || $update)
Use editable config, so save will happen once
Comment #11
segi CreditAttribution: segi commentedHere is the updated patch, based on @andypost feedbacks.
Comment #12
rpayanm@segi you must change the Status to "Need review" for testbot test it.
Comment #13
mon_franco CreditAttribution: mon_franco commentedI have reviewed the last patch, everything seems correct. The only recommendation, after to talk with @tstoeckler about this issue, is to use the object instead of these two functions: language_negotiation_url_domains() and language_negotiation_url_domains_save
Comment #14
mon_franco CreditAttribution: mon_franco commentedComment #15
XanoComment #16
yannickooComment #17
axe312 CreditAttribution: axe312 at Wunder commented- just wanted to remove the hashtag from the issue tag ... yannickoo was faster -
Comment #18
segi CreditAttribution: segi commentedUpdated the patch, I removed call of language_negotiation_url_domains() and language_negotiation_url_domains_save() functions.
Comment #19
segi CreditAttribution: segi commentedComment #20
mon_franco CreditAttribution: mon_franco commentedHi,
Issue reviewed, everything seems correct :)
Comment #21
mon_franco CreditAttribution: mon_franco commentedComment #22
andypostwrong indent
Comment #23
andypostComment #24
segi CreditAttribution: segi commentedI fixed the text indent, but I used this indent because I saw it in the language_negotiation_url_domains_save() and language_negotiation_url_prefixes_save() functions, so the indent wrong there as well.
Comment #25
segi CreditAttribution: segi commentedComment #26
penyaskitoLooks good to me, and still applies.
Comment #27
alexpottLets not do an early return let's wrap the write to config in an
if (!$this->locked() && !$update) {
.Let's just do one config getEditable() instead of the get()s in each of the methods here which will also fix some of the config override bleeding into active config issues.
The other issue can add the test and address the other problems with the procedural functions.
Comment #28
tim.plunkettWhy not just get this config object once (in each method) and manipulate it directly?
Comment #29
segi CreditAttribution: segi at Cheppers commentedI rerolled the patch. My plan is I will continue with the fixes based on feedbacks.
Comment #30
segi CreditAttribution: segi at Cheppers commentedComment #31
Gábor HojtsyWhy are we using getCurrentLanguage() here? We are supposed to react to deleting of possibly multiple entities, not just one AFAIS.
Comment #32
segi CreditAttribution: segi at Cheppers commentedI tried to fix the patch base on feedbacks, but the requests for config management weren't clean for me, so I did a solution. I hope I step closer to final solution.
Comment #33
segi CreditAttribution: segi at Cheppers commentedComment #34
Gábor Hojtsy#2403229: language.negotiation configuration can have overrides bleed in already added the test coverage and resolved the bleeding. This one just cleans up the code now.
Comment #36
Gábor HojtsyTrackertest GET http://ec2-52-88-182-40.us-west-2.compute.amazonaws.com/checkout/user/login returned 0 (0 bytes)
does not seem related at all.Comment #38
Gábor HojtsyComment #41
penyaskitoComment #42
alexpottYes this change makes sense and has no risk but it does not meet our beta guidelines for Drupal 8 inclusion at this time. In my opinion this can go in after Drupal 8 release. I don't think we need to wait to 8.1.
Tagging with a new tag "D8 patch release target" since I don't think we have a tag for this.
Comment #43
andypostsuppose the right tag
Comment #44
Gábor HojtsyRemoving from sprint then.
Comment #49
Berdiryeah.. 8.0 beta is over now :)
Comment #50
andypostreroll + fix unused use
Comment #51
andypostAnd one with cleaner patch
Comment #53
segi CreditAttribution: segi at Cheppers commentedI reviewed the last patch it looks good and the test went well.
Comment #54
segi CreditAttribution: segi at Cheppers commentedComment #55
tstoecklerThis can be written even more succinctly by using
$config->clear('url.prefixes.' $entity->id())
(and the same for the domains).Comment #56
yogeshmpawarUpdated patch as per comment #55 & also added interdiff.
Comment #58
yogeshmpawarmaybe this patch will solve the PHPLint issue.
Comment #60
yogeshmpawarmy bad few things i missed.
Comment #61
BerdirNice. We could save a few more charactes with chained calls, even skip the $config variable completely but not sure if that really is an improvements, lets see what @alexpott thinks.
Comment #62
alexpottAdding credit. The lack of chaining is okay. Makes no difference really.
Comment #63
alexpottCommitted 877e4a8 and pushed to 8.6.x. Thanks!