Comment | File | Size | Author |
---|---|---|---|
#92 | urlgenerator-1946426-92.patch | 1.39 KB | tim.plunkett |
#88 | language-1946426-88.patch | 11.72 KB | tim.plunkett |
#88 | interdiff.txt | 5.2 KB | tim.plunkett |
#87 | 1946426-language-config-form-87.patch | 11.92 KB | pfrenssen |
#87 | interdiff.txt | 4.6 KB | pfrenssen |
Comments
Comment #1
foopang CreditAttribution: foopang commentedI'll take this task :D
Comment #2
foopang CreditAttribution: foopang commentedInitial patch attached, probably need to fix the route access control.
Comment #4
foopang CreditAttribution: foopang commentedFix: Argument 3 passed to Drupal\language\Form\LanguageAdminDeleteForm::buildForm() must be an instance of Drupal\Core\Language\Language, string given
Comment #6
foopang CreditAttribution: foopang commented#4: convert_language_confirm_form-1946426-4.patch queued for re-testing.
Comment #8
foopang CreditAttribution: foopang commentedRemoved trailing whitespace, rerolled the patch.
Comment #9
foopang CreditAttribution: foopang commentedComment #11
foopang CreditAttribution: foopang commentedHelp! I don't see how it can't pass the test.
Comment #13
tim.plunkett#1939660: Use YAML as the primary means for service registration went in, you'll have to make it a language.services.yml now instead of LanguageBundle.
Comment #14
foopang CreditAttribution: foopang commentedOK, I've fixed the patch.
Comment #16
jibran#14: convert_language_confirm_form-1946426-14.patch queued for re-testing.
Comment #18
ramlev CreditAttribution: ramlev commentedHave changed the path, added the Request object as parameter to the delete forms.
Comment #19
Gábor HojtsyTagging with some D8MI tags.
Comment #20
Gábor HojtsyAlso put on sprint.
Comment #21
Gábor HojtsyI assume this is planned to be reused as an Edit access controller in the other related issue :)
drupal_goto() just got removed recently; See https://drupal.org/node/2023537 - you'll need the properly injected version applied to this situation
if you look at the removed code, it uses the procedural approach (post drupal_goto()), this added code is a bit older actually
I also checked the code otherwise but I did not find other outdated snippets in the added code (which is the main danger with patches like this one).
Comment #22
ramlev CreditAttribution: ramlev commentedComment #23
ramlev CreditAttribution: ramlev commentedHave removed the drupal_goto() method and implemented the RedirectReponse.
I have tried to create an interdiff.txt file, but i cant apply the old patch to the code, so it's kinda difficult to create.
Comment #24
Gábor HojtsyI think you'd need the *properly injected* example from the change notice, this is a controller that we are doing the redirection from. You can talk to @katbailey or @YesCT about examples for that if you need more guidance.
Comment #26
ramlev CreditAttribution: ramlev commentedAnnnnd i tihnk i understand DI a bit more now :)
The implementation is "properly injected" now i hope.
Comment #27
Gábor HojtsyThe actual code changes look good, there are missing docs and whitespace.
- All need docblocks.
- The constructor should call the parent constructor as well after the url generator is saved.
- There is extra whitespace before create() that should not be there.
Comment #28
David Hernández CreditAttribution: David Hernández commentedHere we go. Fixed the extra whitespaces, added comments and called parent constructor.
Comment #29
Gábor HojtsyThese should be {@inheritdoc} like the others in the patch.
Is this a container really? I think it is a service, no?
Comment #30
David Hernández CreditAttribution: David Hernández commentedI hope that's fixed now.
Comment #31
David Hernández CreditAttribution: David Hernández commentedUpdated status
Comment #32
Gábor HojtsyAll right, those look good to me. Great collaboration, thanks all!
Comment #34
ramlev CreditAttribution: ramlev commentedHave fixed the bug in the tests, so everything should be ready for merging.
Comment #35
ramlev CreditAttribution: ramlev commentedComment #36
Gábor HojtsyWhy would we need the q there, I don't see.
Comment #37
ramlev CreditAttribution: ramlev commentedI dont either, but can figure out where it get the q from, it's, as fas i have been able to search back, somewhere in either RegionalForm or SystemConfigFormBase classes submitForm methods, because when the form is submitted the q parameter is attached (also through the browser)
Comment #39
ramlev CreditAttribution: ramlev commentedIt seems that the only thing missing right now is to get LanguageTestController test case to work.
The issue here and where im stuck is that the test case test three links created by the language_test module, and these links should have an active class added to the anchor tag when then the correct language is selected, but seems, that no active class is set on anchor tags, dont really know to fix this problem.
Comment #40
webflo CreditAttribution: webflo commentedI am working on it.
Comment #41
ramlev CreditAttribution: ramlev commentedHeres the patch which fixes a few more issues, but missing the LanguageSwitchingTest case fix.
Comment #42
ramlev CreditAttribution: ramlev commentedAnd heres last attempt on the patch.
Comment #43
webflo CreditAttribution: webflo commentedThis patch is based on ramlevs from comment 34.
I had to changed the behaviour for the test. Because language is not an config entity and we dont have upcasting for non entity objects. We will change the behaviour to "Not found" after langauge is a config entity.
Comment #44
ramlev CreditAttribution: ramlev commentedAbout #43, i have made an attempt to fix that in #42 by checking for if the language is existing, if not, throw an 404. By checking for 403 we're getting an access denied, and that could be another issue i think.
Comment #45
ramlev CreditAttribution: ramlev commentedThe testcase which is failing right now is the LanguageSwitchingTest with the 'active' class on anchor tags missing
Comment #47
webflo CreditAttribution: webflo commentedChanged the response code assertion in LocaleTranslationUiTest.
Comment #48
Kristen PolI applied the patch and got this error when trying to delete a language:
on the "Confirm delete" page.
Comment #49
Gábor HojtsyAre you sure you properly applied the patch? We have test coverage for deleting a language and the form ID is provided in the class. Can you try with the patch applied and then a fresh install?
Comment #50
ramlev CreditAttribution: ramlev commentedI have just cloned 8.x from git.drupal.org applied the patch and added the danish language. After that deleted that language with no errors or warnings.
Comment #51
Gábor HojtsyYay, get this in then!
Comment #52
Kristen PolYep. All good with super fresh install (patch applied first)! RTBC++
Comment #53
Gábor Hojtsy#47: convert_language_confirm_form-1946426-47.patch queued for re-testing.
Comment #54
Gábor HojtsyThis will not pass anymore due to the langcode => id property change.
Comment #55
webflo CreditAttribution: webflo commentedRerolled after #1754246: Languages should be configuration entities got it. Changed langcode to id. The interdiff is a diff between to patch files.
Comment #56
andypostRelated #2031277: Implement checkCreateAccess on LanguageAccessController
Comment #57
disasm CreditAttribution: disasm commentedI couldn't find a commit the patch in #55 applied to, so rerolled 47. Attached.
Comment #58
andypostRelated #2031277: Implement checkCreateAccess on LanguageAccessController
Should be EntityControllerInterface because language now config entity
Also please name it LanguageDeleteForm for consistency
Comment #59
vijaycs85Currently blocked by #2031277: Implement checkCreateAccess on LanguageAccessController
Comment #60
penyaskito#2005778: Convert language_admin_overview_form to a Controller landed, including a LanguageAccessController, so I think this is not postponed anymore.
Comment #61
Gábor HojtsyAdd API change tags.
Comment #62
vijaycs85Re-rolling...
Comment #64
vijaycs85Line ending fix...
Comment #66
andypostShould be 404!
Probably special access check should be added to services.yml
Callback - so this item should be removed from hook_menu
Please LanguageDeleteForm
translation should be injected too
suppose $this->entity->delete()
Comment #67
tim.plunkettlanguage_delete() still has its own logic. :(
Comment #68
Gábor HojtsyWe can also do so much at once. I am a big fan of iterative steps and moving forward with changes faster vs. doing everything at once :)
Comment #69
tim.plunkettI meant, we can't fix that here, so don't try to do what #66.5 suggests :)
Comment #70
andypostYes, #66.5 should be ignored
Related - access controlled needs #2031277-11: Implement checkCreateAccess on LanguageAccessController but this change is not needed for delete form so #2003592-16: Convert language_admin_add_form and language_admin_edit_form to a Controllers
Comment #71
pfrenssenGoing to work on this for a bit.
Comment #72
pfrenssenAddressed remarks from #66. I also changed the word "posts" to "content" in the following sentence:
Interdiff is quite large since AdminDeleteForm was renamed to LanguageDeleteForm.
Comment #73
pfrenssenComment #74
jibranWe can move this title to route.yml file now and remvoe this menu item. See drupal_set_title() replaced by returning the title in the render array.
This should be
_entity_form: 'language_entity.delete'
.Sorting order is all wrong.
I think we can use
EntityConfirmFormBase
instead ofConfirmFormBase
Doc block should be according to function definition.
Why are we initializing here with
NULL
?We don't have to set it here. We can set it in
buildForm
.Comment #75
pfrenssenThanks for the review! Will address the remarks.
Comment #76
pfrenssenIf I remove the entry in hook_menu() the help text, local tasks and local actions from the parent menu item appear on the page. This is being discussed in #2049759: Routing within subpath of another route may inherit content from parent route. and @dawehner proposes to leave the router path in hook_menu() for the moment.
But this title was never actually used, instead the title is "Are you sure you want to delete language X?". I will just remove the title from hook_menu().
I improved the definition a bit, but it's not entirely clear to me what you want changed.
Addressed the rest of the remarks.
Comment #77
pfrenssenAdded missing parameter declarations for __construct().
Comment #78
andypostRelated issue #2003592: Convert language_admin_add_form and language_admin_edit_form to a Controllers
language_delete is enough
You need to update lib/Drupal/language/Entity/Language entity annotation for that
there's no usage of entity manager, please remove
could be ignored, but probably will need a string translation service somehow #2059245: Add a FormBase class containing useful methods
else is not needed
Comment #79
pfrenssenThanks a lot for the review, will address the remarks during my flight home this afternoon.
Comment #80
pfrenssenI've addressed all comments except the t() conversions, I worked on this on during a flight and had no internet connection. I could not find any examples of injecting a translation service in the recent commits list. I will do these conversions later today.
Comment #81
jibranThis can be
$this->entity->label();
Comment #82
pfrenssenInjected the translation service, and used label() as suggested by @jibran.
I am still using the "old" translation method as #2059245: Add a FormBase class containing useful methods is not in yet. I think it is rather odd that it is an instance of TranslatorInterface, but is calling the method translate() which is not defined in the interface, but in TranslationManager. This means that we can't swap out this translator with any other class implementing TranslatorInterface, as they might not have the translate() method if I'm not mistaken. That is not for this issue to solve though :)
Comment #83
jibranThis is RTBC for me. Let's wait for @andypost.
Comment #84
andypostI think translation better to revert to
t()
and use it after #2059245: Add a FormBase class containing useful methods because both confirm forms affected please unify them to useaccording to #2003592-24: Convert language_admin_add_form and language_admin_edit_form to a Controllers #3
Drupal/Core/StringTranslation/TranslationInterface is in #2049709: TranslationManager::translate() should be on an interface please remove it
maybe add 'string' type-hint?
Comment #85
tim.plunkettyou can't typehint strings.
Comment #86
pfrenssenThanks for linking the issue about translate() not being on an interface. Nice to see that it got addressed. Will update patch.
Comment #87
pfrenssenAddressed remarks, and made necessary changes to accommodate for #2059245: Add a FormBase class containing useful methods.
Comment #88
tim.plunkettA couple missing tweaks.
Comment #89
jibranAwesome #84 is addressed. Thanks @pfrenssen for finishing this up.
Comment #90
alexpottCommitted eddf21e and pushed to 8.x. Thanks!
This change is approved as part of the meta so removing "api change" tag
Comment #91
tim.plunkettWorking on a fix.
Comment #92
tim.plunkettThis hadn't been retested since #2057155: Add a generateFromRoute() method on the url generator to accept options like url() went in.
Comment #93
webchickRock, thanks!
Committed and pushed to 8.x.
Comment #94
Gábor HojtsyThis was a long time coming. Thanks!
Comment #95
Gábor HojtsySprint tag just does not want to go away :D