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.
Comment | File | Size | Author |
---|---|---|---|
#31 | drupal-1985528-31.patch | 5.54 KB | dawehner |
#31 | interdiff.txt | 555 bytes | dawehner |
#29 | drupal-1985528-29.patch | 5.54 KB | dawehner |
#24 | filter-tips-1985528-24.patch | 5.4 KB | tim.plunkett |
#24 | interdiff.txt | 3.11 KB | tim.plunkett |
Comments
Comment #1
h3rj4n CreditAttribution: h3rj4n commentedAnd the patch
Comment #3
h3rj4n CreditAttribution: h3rj4n commentedRight way of adding new files with a patch.
Comment #4
h3rj4n CreditAttribution: h3rj4n commentedThat one only contains the new files. This one should be the definite one.
Comment #5
h3rj4n CreditAttribution: h3rj4n commentedforgot to set it to 'needs review'.
Comment #7
tim.plunkettIf you want to reuse that method, you'll need
filter_format: ~
under defaults, at the same level as _content.Because your filterTips() method expects something to be passed, or it won't get called.
Leave this off, it should return null.
The
= NULL
is not needed. Also this should be typehinted FilterFormatComment #8
h3rj4n CreditAttribution: h3rj4n commentedI changed everything you pointed out. The only thing I wasn't able to change was the typehinted part.
You mean that I should change the following:
Into
or am I mistaken?
If I change this the function doens't get a value passed along. I think it's because the name of the var isn't the same as the name used in the routing. But if I change the name in the routing I'm not sure if the proper load function is called on the input value.
Comment #9
h3rj4n CreditAttribution: h3rj4n commentedForgot to change the status.
Comment #11
h3rj4n CreditAttribution: h3rj4n commentedThe patch still contains a white pace error ^_^
The patch wont pass the test because a non existing format should return a 404 instead of the current 200.
I think the problem lays inside the Entity load because the access callback is never triggered. I cant find a way to custom send the 404 headers (as a test). I'm guessing that the Entity load fails and stops the code from executing and returning an empty page with headers set to 200 instead of 404.
I couldn't find an example where this already is fixed. I've find the following inside routings.yml files:
If an entity can't be found Drupal returns a 200 instead of a 404. I changed this in the SimpleTest in the supplied patch.
Comment #12
h3rj4n CreditAttribution: h3rj4n commentedComment #14
tim.plunkettThis is blocked on #1987344: NotFoundHttpException thrown in ParamConverterManager results in a WSOD
Comment #15
tim.plunkett#11: drupal-filter_tips_long_changed_test-1985528-11.patch queued for re-testing.
Comment #16
h3rj4n CreditAttribution: h3rj4n commented#8: drupal-filter_tips_long-1985528-8.patch queued for re-testing.
Comment #18
h3rj4n CreditAttribution: h3rj4n commentedThe patch from #8 is the right one.
Comment #19
tim.plunkettI still wish we'd be able to combine these two into one route. Now that those other bugs are fixed, maybe we should try again.
Trailing whitespace
We're not using the
Page callback:
prefix anymore.This should be typehinted with \Drupal\filter\FilterFormatInterface
Remove this
Missing a blank line before the end of the class.
Comment #20
ParisLiakos CreditAttribution: ParisLiakos commentedalso:
should be {@inheritdoc} now
Comment #21
h3rj4n CreditAttribution: h3rj4n commentedAll the documentation I found on an optional variable pointed to a two lines inside the config file. Some of the resources:
http://stackoverflow.com/questions/10091518/symfony2-urls-with-trailing-slash-and-an-optional-parameter
http://stackoverflow.com/questions/11363103/multiple-pattern-in-single-symfony-routing
But after reading some more on the Symphony site I found the following:
Source: http://symfony.com/doc/2.0/book/routing.html
So according to the documentation, this should work for both url's:
But it doesn't. I tried the above code but it doesn't work as symphony says it should work. So I left it to two lines in the routing file.
Comment #22
h3rj4n CreditAttribution: h3rj4n commentedForgot to set it to needs review.
Comment #23
dawehnerThere should be always a new line at the end of the files.
There should be an empty line before the last "}"
We don't inject anything, so let's skip all this code for now.
Comment #24
tim.plunkettCleaned up a bit. The route names were backwards.
Comment #25
dawehnerThis function seems to be a good candidate on the format object.
Comment #26
tim.plunkettWell, we should decide if formats should themselves be checking filter_fallback_format(). If so, we have a lot we could move around.
Comment #27
quicksketchThanks for your work on this guys. Since we'll want to use the routing system for any new paths that need access checks against an individual filter format, this issue has become a dependency for #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms, which needs to grant access to dialogs based on the text format.
Comment #28
alexpottNeeds a reroll now that filters are plugins...
Comment #29
dawehnerRerole.
Comment #31
dawehnerUrgs.
Comment #32
ParisLiakos CreditAttribution: ParisLiakos commentedlooks good
Comment #33
alexpottCommitted b0f09bb and pushed to 8.x. Thanks!
Comment #34
quicksketchYay, that makes this fixed. :)