Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Part of #1971384: [META] Convert page callbacks to controllers
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Comment | File | Size | Author |
---|---|---|---|
#104 | interdiff.txt | 620 bytes | tim.plunkett |
#104 | path-1987802-104.patch | 27.89 KB | tim.plunkett |
#89 | interdiff.txt | 6.93 KB | pfrenssen |
#89 | 1987802-path-controllers-89.patch | 32.88 KB | pfrenssen |
#88 | 1987802-path-controllers-88.patch | 32.86 KB | pfrenssen |
Comments
Comment #1
h3rj4n CreditAttribution: h3rj4n commentedComment #2
h3rj4n CreditAttribution: h3rj4n commentedI think this should work, dunno what I'm doing wrong.
Comment #3
vijaycs85Comment #5
laurentchardin CreditAttribution: laurentchardin commentedFixing namespace + return static on ControllerInterface::create()
Comment #7
dawehnerJust some general comments which could be improved.
Just to be sure to include the @return on the next rerole.
This could use an injected database connection
truncate_utf8 could use the Unicode class
So what about inject the alias manager :)
Please don't kill the weight.
Comment #8
laurentchardin CreditAttribution: laurentchardin commentedHere is a more revamped patch.
Updated:
path.alias_manager vs path.alias_manager.cached
?New:
Issues:
$whitelist = cache()->get('path_alias_whitelist');
does not seem to return anything, making thetestPathCache()
fails. I could use some help here.Comment #9
laurentchardin CreditAttribution: laurentchardin commentedRerolling patch.
Comment #10
dawehnerAlias manager have an interface so let's use it
This docs should have the full namespace added at the front.
See #1858196: [meta] Leverage Symfony Session components so let's adapt the $_SESSION
Should also use $this->database
This bits can be replace by {@inheritdoc}
The exact pattern in {@inheritdoc}
There is no reason to move this bit around.
Comment #12
laurentchardin CreditAttribution: laurentchardin commentedRerolling patch.
Comment #13
laurentchardin CreditAttribution: laurentchardin commentedForgot to change the status.
Comment #14
dawehnerManual testing of the page works fine.
Let's talk about path :)
Please add a linebreak at the end of the file.
Comment #15
laurentchardin CreditAttribution: laurentchardin commentedDamn... nice catch dawehner !
Here's an update.
Comment #17
dawehner#15: drupal-1987802-15.patch queued for re-testing.
Comment #18
dawehnerThere we go
Comment #19
laurentchardin CreditAttribution: laurentchardin commentedChanging state because we need to merge this one with #1987800: Convert path_admin_edit() to a new style controller
With the support of MENU_LOCAL_TASK, introduced by #1908756: Separate Action Links (MENU_LOCAL_ACTION) from hook_menu(), those 2 issues are interdependent.
Also see https://drupal.org/node/2007444
We need the path_admin_overview route to support the add button local task for the path_admin_edit one.
I proposed on IRC to merge both and work on an unique patch, as it will make review and testing easier.
Comment #20
laurentchardin CreditAttribution: laurentchardin commentedHere we are.
Summing-up:
Tests:
Feebacks needed:
path.alias_manager
orpath.alias_manager.cached
?** Updated ** :
Comment #21
laurentchardin CreditAttribution: laurentchardin commentedAdding tag as mentioned on #1971384: [META] Convert page callbacks to controllers
Comment #22
dawehnerRegarding the service: use the cached version of it.
Comment #23
Crell CreditAttribution: Crell commentedThis should be moved to a new method on the Path service. There's no good reason for SQL in a form, certainly not in the validate method.
Comment #24
disasm CreditAttribution: disasm commentedcleanup of controllers and forms.
Moved query to aliasManager service.
Comment #26
tim.plunkettMissing blank line
There is a moduleHandler() method on ControllerBase
Does this even work? setContainer() isn't called until afterward I beleive.
Missing .
Bad indentation
This definitely doesn't work... FormBase implements ControllerInterface, use that.
Please split this back into two methods, this is really confusing and hacky.
?
These @todos should be proper sentences (here and throughout the patch)
This doesn't work anymore. It needs to be a YAML file
Comment #27
disasm CreditAttribution: disasm commentedAddressing most of #26.
Comment #29
disasm CreditAttribution: disasm commentedaccidentally amended my commit, so interdiff is to #24. This should resolve all the test failures. I've manually tested it, and all is functioning.
Comment #30
jibranDoc block needs update.
We are not adding any tags why not query instead of select.
@peram missing in doc block.
We can use query here?
I think we have to add \Drupal.
Maybe we can add use statement on top.
We can replace l.
We can't mix these two.
hmmm
Injection
issue link maybe or we can remove this line.
FormBase
$this->t.
Comment #31
disasm CreditAttribution: disasm commentedHandling most of issues in #30. #2 and #4, lets handle in a follow-up (trying to minimize the refactor). #6, what does l() become in a class? I don't see an equivalent method on UrlGeneratorInterface. #9 lets handle in a follow-up.
Comment #32
disasm CreditAttribution: disasm commented$this->l() is part of ControllerBase. Fixing.
Comment #33
jibranWhy we need follow-up other then these minor issues?
Typo
Comment #34
disasm CreditAttribution: disasm commentedFixing typo, adding urlGenerator to EditForm.
Comment #35
disasm CreditAttribution: disasm commentedFixing autocomplete mistake.
Comment #37
tim.plunkettThis doesn't work like this. It is a string, the full namespaced class must be passed to extend. It's the same as the difference between
instanceof
andis_subclass_of
Missing @param
Weird blank line, but also $pathAlias needs to be $path_alias
$this->t()
Missing blank line
Let's do
$request = $this->getRequest();
firstDouble blank line
{@inheritdoc}
Let's fix this wrapping
Please just specify two #submit methods:
'#submit' => array(array($this, 'submitFilter'))
'#submit' => array(array($this, 'submitReset'))
And then remove the $op checking and switch/case by having two methods.
?
Comment #38
disasm CreditAttribution: disasm commentedattached patch addresses comments in #37.
Comment #40
tim.plunkettFixed a bunch of stuff. @disasm, you have a very bad habit of mismatching your case on object properties! :)
Comment #41
dawehnerAny reason we don't use a db_and?
where => condition
Do we really still need this?
Comment #42
xjmThanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.
Comment #43
plopesc@disasm let me know about this issue.
New patch addresses the
condition()
anddb_and()
suggestions.About the
hook_menu()
changes, I'm not sure about the desired structure. I tried to remove the whole reference to this path and the title page was removed. Removing only #type and #weight, title worked, but it displayed the help message defined foradmin/config/search/path
instead ofadmin/config/search/path/add
.Any suggestion?
Thanks in advance
Comment #44
jibran#43: path-1987802-43.patch queued for re-testing.
Comment #45
jibranNow we can use $this-> urlGenerator() and remove this.
Please add an issue and reference it here.
Comment #46
disasm CreditAttribution: disasm commentedFixed urlGenerator(). Removed the todo, because it really makes no sense. Currently, The pid is passed to a load function on a CRUD service that is returning an associative array with some properties, like alias, language, etc... This cannot be upcasted in it's current state because there is no class to Upcast it to. Ideally, we should completely remove the Path CRUD service, create a model for a PathAlias with the attributes required, and then this model could be upcasted based on the pid, but this is so out of scope for this issue, it's ridiculous.
Comment #48
disasm CreditAttribution: disasm commented#46: drupal8.path-module.1987802-46.patch queued for re-testing.
Comment #49
jibranMinor issues.
Not needed anymore.
\Drupal
Comment #50
disasm CreditAttribution: disasm commentedLeaving #2 alone for now. Did a grep of core, and every instance of ->extend does not have a leading \ before Drupal. #1 is addressed. Also, not in the interdiff because it was part of reroll, but routes are now path.
Comment #51
jibranPatch needs reroll after #2051097: Change "pattern" to "path" in *.routing.yml files.
Comment #52
Berdir#50: drupal8.path-module.1987802-50.patch queued for re-testing.
Comment #54
pfrenssenRerolled. The routes were already created in #2089635: Convert non-test non-form page callbacks to routes and controllers with different identifiers. I retained the identifiers from the previous patch. I also renamed path.delete to path.admin_delete to be consistent with the others.
Comment #56
pfrenssenWorked some more on this.
The "Add alias" local action is shown on the delete alias confirmation form. It's not clear to me how I can prevent it from showing up there.
Comment #57
jibranWe can make this db_query.
Needs @params
\Drupal
We can't mix these.
Filters form submission handler.
Resets form submission handler.
Comment #58
dawehnerLet's adds some lines of documentation on here.
Lazy copy and paste of existing documentation ...
No double braces needed.
I don't care much but we could store $this->getRequest()->query as local variable, so the code might gets a bit easier to read.
I don't know how to document a form constructor with additional parameters either.
Oh, what is the reason to remove the _title from here?
Here is an interdiff which solves the problem you described, thank you for mentioning it, this is an actual bug in core.
Comment #59
jibranx-post.
Comment #60
pfrenssenComment #61
pfrenssenFrom #57:
Do you mean changing it to a $this->connection->query()? What would be the benefit?
From #58:
_title does not work with _form. These routes were initially converted to _content placeholders in #2089635: Convert non-test non-form page callbacks to routes and controllers which do support _title. The titles are being output in
$form['#title']
. I understand the reasoning behind this, as a form is not strictly linked to a route but can also be placed arbitrarily on a page, and needs to be able to provide a title regardless of the route used.Comment #62
pfrenssenWoops forgot to add dawehner's patch that fixes the stray local action. Here it is.
Comment #63
jibranIt is bit faster then
$this->connection->select()
.select
is normally used for alterable queries.Parent method has no pid param so we have to document all the params here with function description.
This change is not required.
Comment #64
pfrenssenComment #65
pfrenssenAdressed remarks.
Comment #66
dawehnerWhile manually testing I realized that the source column also contains the path alias but this is not the problem of this issue: #2096135: PathProcessorAlias ignore 'alias' => TRUE
Comment #67
jibranAwesome @pfrenssen thanks for fixing the issue. +1 for RTBC.
Comment #68
alexpottPatch no longer applies.
Comment #69
tim.plunkettThe menu.inc hunk went in elsewhere, the other conflict was translation module being removed.
Comment #70
jibranBack to RTBC.
Comment #71
alexpottPatch no longer applies.
Comment #72
XanoRe-roll.
Comment #74
vijaycs85Another re-roll from #69
Comment #75
jibranBack to RTBC.
Comment #76
Xano#74: 1987802-path-controllers-74.patch queued for re-testing.
Comment #78
Xano#74: 1987802-path-controllers-74.patch queued for re-testing.
Comment #79
dawehnerNitpick: Should be FALSE here.
I don't think we should call that connection service. This is the database connection.
We mostly use " =& " as code style here.
Let's not forget the empty line here.
Comment #80
Berdir"We mostly use " =& " as code style here."
No, quite sure that "= &$..." is correct, to better separate it from &=. Search for e.g. "= &drupal_static" vs. the other way round.
Looks like a lot of the instances that use the other way in core are part of views code...
Comment #81
dawehnerOh right, I just thought that > 60 instances of " =& " would be a lot. But I disagree with sticking the "&" to the right expression
in order to show the reference.
Comment #82
tim.plunkettBoth path_admin_filter_form_submit_filter() and path_admin_filter_form_submit_reset() are broken in HEAD, and this papers over that we have no test coverage. Either we open an issue to fix that and block this one, or just do it here.
Comment #83
vijaycs85Re-rolling with fix for #79. Still needs test, just setting review for testbot...
Comment #86
vijaycs85Fixing fails...
Comment #87
jibranLooks great only minor doc issues.
I think we can expand it to 80 chars.
Full stop missing :). I think Implement should be with small i.
Why not just database?
The database connection service.
I think we should restore the original desc
Comment #88
pfrenssenGoing to address the above remarks. Here is already a straight reroll to latest HEAD.
Comment #89
pfrenssenAddressed the remarks from #87. Also went over the patch with a fine comb and straightened out some last minor kinks.
The first word following a @todo ("Implement") should be capitalized according to coding standards, but the link should be on a separate @see line. Fixed it.
Comment #92
andypost89: 1987802-path-controllers-89.patch queued for re-testing.
Comment #94
fgmRerolled on current HEAD. Main changes:
Comment #96
fgmThis should remove most errors, though probably not all.
Comment #98
fgmPath::__construct() must type hint on PathInterface now that it exists, not Path. This is needed for the MongoDB service override.
Comment #100
tim.plunkettWill work on this and the tests.
Comment #101
tim.plunkettPostponing this on #2244757: Path Admin UI is broken and has no test coverage
Comment #102
tim.plunkettOpened #2244861: Four public methods on \Drupal\Core\Path\AliasStorage aren't on the interface. Not a blocker, but its stranger now that we're typehinting.
Comment #104
tim.plunkettHAH.
Comment #105
tim.plunkettComment #107
andypostMuch more cleaner.
minor, seems better to use else
Comment #108
kim.pepper104: path-1987802-104.patch queued for re-testing.
Comment #109
alexpottCommitted 93e7749 and pushed to 8.x. Thanks!
Comment #111
tim.plunkettThanks!