Problem/Motivation
This issue has been promoted to critical because it also resolves #2324809: If entity canonical path is overridden with a view with optional arguments, content translation's routes will result in fatal errors (in addition to being a beta target and part of the scope of the VDC initiative).
Similar to #1806334: Replace the node listing at /node with a view. The default taxonomy term listing page is inflexible, and Views has long shipped with a replacement.
Proposed resolution
- Remove the custom code and use the view.
- Remove the depth argument from the default view for a more performant page.
Queries before conversion
SELECT COUNT(t.nid) AS expression FROM taxonomy_index t WHERE (tid = :db_condition_placeholder_0)
SELECT t.nid AS nid, t.tid AS tid, t.sticky AS sticky, t.created AS created FROM taxonomy_index t WHERE (tid = :db_condition_placeholder_0) ORDER BY t.sticky DESC, t.created DESC LIMIT 10 OFFSET 0
Query after conversion
SELECT taxonomy_index.sticky AS taxonomy_index_sticky, taxonomy_index.created AS taxonomy_index_created, node.nid AS nid, node_field_data.langcode AS node_field_data_langcode FROM node node INNER JOIN taxonomy_index taxonomy_index ON node.nid = taxonomy_index.nid INNER JOIN node_field_data node_field_data ON node.nid = node_field_data.nid WHERE (( (taxonomy_index.tid = :taxonomy_index_tid) )) ORDER BY taxonomy_index_sticky DESC, taxonomy_index_created DESC LIMIT 11 OFFSET 0
Per @dawehner, the node JOIN in this query is unnecessary, and an as-yet unfiled followup issue for #1740492: Implement a default entity views data handler can enable views to rely on the field data table without needing the base table when possible.
Remaining tasks
User interface changes
The feed icon is no longer visible on term pages to access the taxonomy term feeds. (It was already broken in D8 HEAD for awhile, linking to the wrong path.) This will be resolved by:
- #2251121: Support to add a feed icon on pages with parameters
- #2251861: Add the feed icon back to taxonomy/term/% pages
API changes
- taxonomy/term/% pages are replaced with a view.
- The following callbacks/page builders are removed:
- taxonomy_term_page()
- taxonomy_term_feed()
- node_feed()
- TaxonomyController::termPage()
- TaxonomyController::termFeed()
Original report by @tim.plunkett
Comment | File | Size | Author |
---|---|---|---|
#216 | vdc-1857256-216.patch | 40.23 KB | dawehner |
#216 | interdiff.txt | 1.01 KB | dawehner |
#209 | vdc-1857256-206.patch | 40.17 KB | Gábor Hojtsy |
#206 | interdiff.txt | 1.58 KB | dawehner |
#206 | vdc-1857256-206.patch | 40.17 KB | dawehner |
Comments
Comment #1
tim.plunkettSee attached. 27 insertions(+), 125 deletions(-) is nice.
Comment #3
tim.plunkettWell taxonomy_term_page() had some really hacky code for breadcrumbs, I've done an equally hacky job at porting that.
Comment #4
dawehnerYou know my friend, learn the power of views.
Comment #6
dawehnerSee #1857376: Provide an area handler that renders an entity for the entity area handler.
Comment #7
dawehnerSummary of the test failures:
Comment #9
dawehnerGot the breadcrumbs running, so it's just a matter of getting the entity area rendering and configure it here.
Comment #11
dawehner#9: drupal-1857256-9.patch queued for re-testing.
Comment #13
xjmThis is part of #1864980: [meta] Figure out how to integrate Views into core.
Comment #13.0
podarokadd meta
Comment #14
dawehnerAgainst, this issue is currently blocked on #1857376: Provide an area handler that renders an entity
That's just a rerole.
Comment #15
tstoecklerThis is another re-roll. Haven't tried it out yet.
Comment #16
dawehnerSet to needs review to see what errors are missing.
Comment #18
dawehnerLet's use the new area handler now.
Comment #19
ParisLiakos CreditAttribution: ParisLiakos commentedJust tried this, seems though that we need to get rid of title in the header view area of the term. See screenshots
really minor: the comment needs some refining..
Comment #20
tstoecklerI suppose we could add a setting to the area handler that suppresses the title. It seems that not only taxonomy terms could profit from that. Maybe we should only show that setting for the 'full' view mode? Hmm...
Comment #21
dawehnerHaving a $page variable seems to be a concept which exists on different kind of entities (both node and taxonomy have it).
Now that taxonomy/term is a view, taxonomy_term_is_page doesn't seem to make any sense anymore, as you really want to have full control over it.
I'm wondering whether there is a plan to make the label configurable on the display bundle settings? Wouldn't this basically allow the site builder to create a
display format, which don't contain the label?
Comment #22
ParisLiakos CreditAttribution: ParisLiakos commentedThat would require terms going NG first.
why dont we make taxonomy_term_is_page() work with views for now?
Comment #23
dawehnerWell, as you can't use it with menu_get_object() anymore, it seems to be that you need to use arg() or get the
argument from the current request object. Does someone has a better idea?
Comment #24
ParisLiakos CreditAttribution: ParisLiakos commentedhow about using
menu_base_path
from Term Enity plugin annotation?Comment #25
dawehnerWell, yeah we could remove %taxonomy_term from this string and then break again, once people realize that this feature doesn't work together with route items, mh.
Comment #26
xjmRelated: #1341720: Add per-vocabulary settings for listing pages
Comment #27
dawehnerSee #1818560: Convert taxonomy entities to the new Entity Field API as a blocker.
Comment #28
andypostBy using taxonomy_menu module for D7 I got a lot of problems with performance caused by the way views arguments.
So for each menu router overriden by view I got access check that initializes view to check access
Comment #29
dawehnerWell, what should do views, not validate the stuff? In Drupal 8 once #1800998: Use route system instead of hook_menu() in Views is in, we might should see
whether we can move validation to the route level as well, so we don't need an actual view instance.
Comment #30
ParisLiakos CreditAttribution: ParisLiakos commentedretitling to include the feed, i just closed the two conversion issues for this one
Comment #31
trevjs CreditAttribution: trevjs commentedComment #32
rdrh555 CreditAttribution: rdrh555 commentedComment #33
rdrh555 CreditAttribution: rdrh555 commentedEnable the Taxonomy term view and modified the Page display at Path: /taxonomy/term/% .
Original view was using mini-pager and showed "Page 1" at the bottom when there was only one item to show. Changed the pager to "full" and tested. Pager now displays numbers and there is no pager on a page with 1 item.
Comment #34
dawehnerThanks for testing it.
There has been an issue for that: #1998330: Minipager is broken on page size == 1 due to ceil(PHP_MAX_INT / 1)
Comment #35
Berdir#18: drupal-1857256-18.patch queued for re-testing.
Comment #37
tim.plunkettThis was related to #2011006: Default local tasks provided by Views are broken
Comment #39
tim.plunkettMissed one change.
Comment #41
tim.plunkettIt's getting the parent menu item's link title instead of the term. I feel like we fixed this before...
Comment #42
jibranPostpone on this #2026081: Clean up ForumBreadcrumbBuilder::build().
Edited: Actually #2026081 is postpone on #1951268: Convert /forum and /forum/% to new router, remove forum_forum_load(), forum_get_topics(), create Forum service sorry for the noise.
Comment #43
klonos...coming from #1341720: Add per-vocabulary settings for listing pages
Comment #44
ekes CreditAttribution: ekes commentedCombined patch from changes made (and still required, or otherwise not already applied) in #18 corrected to apply to head, with #39.
Additional change to account for legacy breadcrumb system no longer having any content by default. Later #2026073: Convert drupal_set_breadcrumb/ViewExecutable::getBreadcrumb() to breadcrumb builder service in ViewExecutable could remove any dependency on the legacy system.
Comment #45
ekes CreditAttribution: ekes commentedComment #47
ekes CreditAttribution: ekes commentedMissing the moved file from the patch. Try again - Sorry.
Comment #49
tim.plunkettPretty sure this is causing fails.
Is this really needed?! :(
Comment #50
ekes CreditAttribution: ekes commentedThere are two reasons the the tests are failing:-
First. The present hook_menu() item that is being replaced does an entity access view check for the access callback. The view adds an access content permission check; but doesn't have a entity access check. Two solutions suggested:
Second. Even with access to the page. The test requires the title (parent item) to be the name of the term. The view is now setting the parent item rather than the hook_menu item. This doesn't have replacement for the entity (it is presently hard coded to 'Taxonomy term'). Suggested options: set page title from the hook_menu for taxonomy/term/%taxonomy_term/edit; or if possible enable 'parent item' to be able to do entity %1 replacements.
Comment #51
dawehnerI would really go with the drupal_set_title() idea for now. This is the way to make progress
Comment #52
xjmCrossposting: #1830588: [META] remove drupal_set_title() and drupal_get_title().
Comment #53
dawehnerSubissue to provide a route: #2062769: Convert taxonomy edit form to a route
Comment #54
jibranRelated #2061911: Remove drupal_set_breadcrumb and LegacyBreadcrumbBuilder in taxonomy module and #2061913: Remove drupal_set_breadcrumb and LegacyBreadcrumbBuilder in Views module.
Comment #54.0
jibranCorrect meta issue.
Comment #55
dawehnerStill blocked on #2110845: Allow overriding views to override paths with parameters
Comment #56
klonosComment #57
pcambraHere's a re-roll of [#47] to see what's the shape of this after some months of changes
Comment #59
pcambraAfter discussing with @dawehner on IRC, we're reverting the changes that remove the fallback listings as it seems is an easier path. Taxonomy routing elements (for page at least) are too coupled to just remove them like that.
The possible paths seems to be keep the listing and solve the comments in #50 to implement the breadcrumb or allow Views UI to have a new setting to set the route name (if we go this way we'd need a separate issue).
New re-roll of the patch fixing the above stuff and some other minor things.
Comment #60
dawehnerOpened some issues for the described UI features: #2138667: Allow to specify the route name for a path-based view #2138665: Allow to specific route parameters to be defined via the views UI
Comment #62
vijaycs85Re-rolling, seems lots of clean up when in as part of #2061913: Remove drupal_set_breadcrumb and LegacyBreadcrumbBuilder in Views module
Comment #64
dawehnerA couple of bugs in Entity and views:
Comment #66
vijaycs8564: vdc-1857256.patch queued for re-testing.
Comment #68
dawehner64: vdc-1857256.patch queued for re-testing.
Comment #71
star-szrTagging for reroll.
Comment #72
rhabbachi CreditAttribution: rhabbachi commentedComment #73
InternetDevels CreditAttribution: InternetDevels commentedUnfortunately for me I've created a patch yesterday, but haven't posted it because interdiff looked suspicious. I've seen that you've tagged this issue for your sprint, but decided to post my patch anyway, maybe it can be useful for you.
Comment #74
dawehnerThank you for rerolling the patch but please don't be dissapointed if it gets red.
Comment #76
rhabbachi CreditAttribution: rhabbachi commentedComment #77
rhabbachi CreditAttribution: rhabbachi commentedComment #79
hussainwebI rebased this from #64. I checked the diff's carefully after InternetDevel's comment in #73. I hope the reroll works.
Comment #81
hussainwebAttaching a bugfix. The patch used $entity_info as an array but it is supposed to be an object (it might have changed later). I did manual tests and the term pages loaded fine. I see that it is using views from the generated markup.
I also generated a diff with -M switch to detect rename of the view YAML file. Hence, the patch is much smaller.
Comment #82
hussainwebComment #83
tim.plunkettWhy is this part of this patch? This looks similar to the changes in #2167641: EntityInterface::uri() should use route name and not path, but only halfway.
Comment #84
hussainwebIt was a part of the patch in #64. dawehner mentioned there were some bugs in Entity and Views implementation and I just assumed they were fixes, though, like you said, should be part of a different issue. Should we block this until that patch goes in?
Comment #86
hussainwebI see the failures are probably related to the changes mentioned in #83. I think we will have to wait for #2167641: EntityInterface::uri() should use route name and not path here?
Comment #87
hussainwebThe reason for the failure is this:
For
$rel == 'admin-form'
, the route is/admin/structure/types/manage/{node_type}
. And{node_type}
is not available down the line inSymfony\Component\Routing\Generator\UrlGenerator::doGenerate
.I don't think the issue should be tackled here. Should we wait until this is fixed in #2167641: EntityInterface::uri() should use route name and not path? I will try later without these parts if I get a chance and see what is the bug.
Comment #88
hussainwebSince #2167641: EntityInterface::uri() should use route name and not path is in, lets try again.
Comment #90
catch#2102485: Remove drupal_set_title in taxonomy module controllers is currently postponed on this, and that in turn blocks #1830588: [META] remove drupal_set_title() and drupal_get_title(). I don't think there's a real dependency, but since there's active work here and not on the postponed issue, bumping to critical. If this turns out to be difficult to get done, we should consider unpostponing the other issue though instead (since it's the very last drupal_set_title() in core).
Comment #91
ianthomas_ukBack to normal, as #2102485: Remove drupal_set_title in taxonomy module controllers has actually already been fixed.
I had a look over the patch and the failing tests though:
- DefaultConfigTest failure is easy, just needs '1' changed to true on line 5 of views.view.taxonomy_term.yml.
- PathTaxonomyTermTest is failing because the description isn't showing on the view term page (nor is the rss link).
- We've moving the contents of taxonomy_term_page and taxonomy_term_feed into TaxonomyController. Wouldn't it be better to do that in a separate issue, so that any logical changes to the contents of those functions are easier to review?
- TaxonomyController::termPage() reintroduces a call to drupal_add_title. It's probably using an older version of the function and changes got lost in re-rolls.
- Why do we even have TaxonomyController::termPage() and termFeed()? Isn't the point of this issue to remove them?
Unassigning from rhabbachi as other people have been working on patches, but feel free to re-assign if you're actively working on this.
Comment #92
sunWould be great to move forward here, since this conversion allows us to remove a whole bunch of legacy procedural API functions throughout core (
taxonomy_term_feed()
→node_feed()
→format_rss_*()
, etc.)Comment #93
dawehnerLet's see how much is broken.
Comment #95
xjmComment #96
dawehnerWe are getting there.
Comment #98
dawehnerComment #99
xjmComment #101
dawehnerNow we just need the schema issue.
Comment #103
vijaycs85Let's postpone this as the 4 failures are fixed by #2246557: Missing schema for entity area plugin
Comment #104
jibran101: taxonomy_term-1857256-101.patch queued for re-testing.
Comment #105
jibran#2246557: Missing schema for entity area plugin is in.
Comment #107
tim.plunkettYay for schemas!
Comment #108
xjmGreen patch, happy happy.
Did some manual testing.
All the contextual links and such work fine.
When Views is uninstalled, it just falls back to just rendering the entity with no listing:
Next a code review.
Comment #109
xjmMmm, is "term with depth" the right contextual filter? I don't think HEAD supports a depth argument, does it? and this would also change the feed paths, right?
Speaking of, we also need to add the feed icon to the view.
Not that that link works in HEAD... you get
...So good thing that we're removing it. And sounds like we also need to add some test coverage for the feed display.
Comment #110
xjmOh... hm. I'd be in favor of removing this and requiring views for the feed.
Edit, re-edited: There's no fallback provided for the node feed without Views enabled in HEAD, which I think is good. So I wonder if we can remove
node_feed()
after we convert this?Comment #111
dawehnerschnaps-number!
In Drupal 6 core had that feature, and got removed in D7 with the promise that views implements it.
That is the reason why the d7 view which is basically still the same as in d8 has that feature. I really think we should keep it.
Comment #112
ParisLiakos CreditAttribution: ParisLiakos commentedlets also get rid of node_feed and resolve that todo there? (EDIT: actually nvm, cause that means we would have to also remove format_rss_channel and probably more..so lets do it in a followup)
Also, shouldnt the feed be attached to the page display, so that we have the feed icon in the page?
Comment #113
dawehnerOpened a follow up: #2251111: Remove format_rss_item() and format_rss_channel()
Comment #114
dawehnerWe can't do at the moment, #2251121: Support to add a feed icon on pages with parameters is a follow up.
Comment #115
ParisLiakos CreditAttribution: ParisLiakos commentedthanks for opening those follow ups!
Comment #116
ParisLiakos CreditAttribution: ParisLiakos commentedComment #117
xjm+1 on the RTBC and the followups.
This needs a change record; I'm working on it.
Comment #118
xjmAlrighty, change record is set. :) Added to https://drupal.org/node/2084727.
Comment #119
xjmJust a couple little comment fixes.
Comment #121
xjmWhoops, fixed the wrong comment. Being a little naughty and left in the other fixed comment as well so that they're the same. :P ...And reuploading the right thing.
Comment #123
xjmComment #124
catchThe depth argument was taken out of the taxonomy/term view in core at least partly due to performance. We shouldn't support that in the default view for the same reason. It's still 'provided by views' either way.
Could someone show the raw query or an EXPLAIN before and after on the term pages to ensure there's no regression in the query?
Comment #125
xjmComment #126
oadaeh CreditAttribution: oadaeh commentedThis is simply a re-roll of @xjm's patch to account for the recent core changes. None of the issues raised have been addressed. All tests pass locally.
Comment #127
ParisLiakos CreditAttribution: ParisLiakos commentedOK, i created 3 terms
test
-test1
--test2
Visiting the test2 term page as user 1:
Before:
SELECT t.nid AS nid, t.tid AS tid, t.sticky AS sticky, t.created AS created FROM taxonomy_index t WHERE (tid = :db_condition_placeholder_0) ORDER BY t.sticky DESC, t.created DESC LIMIT 10 OFFSET 0
After:
SELECT node_field_data.sticky AS node_field_data_sticky, node_field_data.created AS node_field_data_created, node.nid AS nid, node_field_data.langcode AS node_field_data_langcode FROM node node INNER JOIN node_field_data node_field_data ON node.nid = node_field_data.nid WHERE (( (node_field_data.status = 1 OR (node_field_data.uid = 1 AND 1 <> 0 AND 1 = 1) OR 1 = 1) AND (node.nid IN (SELECT tn.nid AS nid FROM taxonomy_index tn WHERE ( (tn.tid = :db_condition_placeholder_0_0) ))) )) ORDER BY node_field_data_sticky DESC, node_field_data_created DESC LIMIT 11 OFFSET 0
I guess the real difference here is the subquery. Also you can see that views cares for sticky field, but you can easily remove it.
as far as the depth is concerned, it is the same.
it only accounts for depth if you add the path argument for that. eg
taxonomy/term/1/2
where the query looks like:
i guess back to catch?
Comment #128
dawehnerJust changed the file by hand.
Comment #129
ParisLiakos CreditAttribution: ParisLiakos commentedbot can object if it wants to
Comment #131
dawehnerHAHA, what a fail!
Comment #133
dawehnerIt is SO funny to see that even the config team don't give a fuck about config schema.
Comment #135
dawehner133: vdc-1857256-133.patch queued for re-testing.
Comment #136
ParisLiakos CreditAttribution: ParisLiakos commentedComment #137
catchThat subquery doesn't look encouraging. If we can demonstrate that's not a problem (via a large dataset + EXPLAIN) then fine, but more likely we'll need to refactor to remove that I think.
Comment #138
dawehnerWe do not longer use the subquery, as we removed support for the depth again.
Comment #139
ParisLiakos CreditAttribution: ParisLiakos commentedyes, there is no longer a subquery. just manually tested it to make sure:
Now that the depth was removed, the subquery is no longer there.
back to catch
Comment #140
catchSorry I missed this first time 'round but it also concerns me.
Is this the 'published or admin' filter or similar? Whichever it is it means the caching granularity for the view (as well as the MySQL query cache) becomes per-user.
If we make that just published agreed this is RTBC.
Comment #141
ParisLiakos CreditAttribution: ParisLiakos commentedYes, its indeed the "published or admin"
Good point! switched it to just the "Published" filter. here is the query now
Comment #142
catchSorry each time the query gets better, I spot something else...
{taxonomy_index} only includes published nodes - the whole point of that table is to avoid the conditions and/or sorts across two tables. For the same reason it has sticky and created columns denormalized from node. So we ought to be able to 1. remove the status filter 2. have the ORDER BY on that table.
That would get us back more or less to this:
.
Comment #143
ParisLiakos CreditAttribution: ParisLiakos commentedtbh i never noticed that
taxonomy_index
table had created and sticky fields:)new query:
If the langcode is removed then there is one less join the one to
node_field_data
but i am not sure if its a good ideaComment #144
ParisLiakos CreditAttribution: ParisLiakos commentedComment #145
catchWhat's adding the langcode to the query?
Comment #146
ParisLiakos CreditAttribution: ParisLiakos commentedi am not sure, but it has something to do with the display plugin, since the feed's query doesnt include a langcode and looks like this
Comment #147
BerdirYeah, that's the display with the setting that allows you to control how to deal with translations, I think that is the default setting that displays the node in every translation. I *think* that switching to displaying it in the current UI language would get rid of that and it would make more sense. But that is the same behavior as the frontpage has now (which has an issue to change it).
Comment #148
dawehnerI guess berdir is talking about #2161845: Node views (front page, admin) do not use the proper language filter
Given that the expected query in #1857256-142: Convert the taxonomy listing and feed at /taxonomy/term/%term to Views can't really be achieved I wonder what we can do here or just skip the conversion and live with it.
Comment #149
ParisLiakos CreditAttribution: ParisLiakos commentedhopefully this moves on.
here is a reroll for now
Comment #150
dawehnerWe also have to take into account that currently HEAD does not use an entity query, so it is not really a fair comparison
Comment #153
hussainwebJust helping with a reroll. I think I am out of touch on this issue. I will try and look once it goes through testing.
Comment #157
jhodgdonI have not read the 156 comments here to see if this has already been addressed... But please see #2324809: If entity canonical path is overridden with a view with optional arguments, content translation's routes will result in fatal errors -- make sure to test this with the Language module enabled, because the current taxonomy view doesn't work with Language enabled.
Comment #158
dawehnerReroll for now.
Comment #160
jhodgdonNote: the error on the other issue is with Content Translation enabled, not just Language.
Comment #161
dawehnerLet's see how much fails are fixed by that.
Comment #162
jibranI think we should add the tests added in #2324809-10: If entity canonical path is overridden with a view with optional arguments, content translation's routes will result in fatal errors to this patch because there is no tests for the taxonomy term view page and its translation.
Comment #164
dawehnerLet's see whether this fixes is already.
@jibran
Do you want to merge the tests?
Comment #165
jibranI am bumping this critical and bug according to #2324809: If entity canonical path is overridden with a view with optional arguments, content translation's routes will result in fatal errors because the test fails in that issue are fixed by the changes made in this patch and closing that as a duplicate of this. 2324809-test-only.patch should be green here.
Comment #166
jibranThis is a complete patch with fixes from #164 and tests form #2324809-10: If entity canonical path is overridden with a view with optional arguments, content translation's routes will result in fatal errors. Sorry for the x-post and noise.
Comment #169
dawehnerThe testname and the documentation should explain that this ensures that the view is working fine together with translations.
Comment #170
larowlanQuick review, more later
needs scope modifier public
Why not add these to protected static $modules?
Comment #171
Gábor HojtsyI see this resolves the fatal from #2324809: If entity canonical path is overridden with a view with optional arguments, content translation's routes will result in fatal errors by removing the depth argument currently in the view (but that is not THE reason to remove the depth argument, so yay for yet another good side effect), but #2324809: If entity canonical path is overridden with a view with optional arguments, content translation's routes will result in fatal errors is not resolved for the case when someone does want to do that or override other entity pages with views that may have more dynamic arguments. So I think #2324809: If entity canonical path is overridden with a view with optional arguments, content translation's routes will result in fatal errors is still worth reopening on a lower priority level.
Comment #172
dawehnerWell, I just don't want to enable views for all the tests.
Comment #173
Gábor HojtsyLooks good except #170/1 and the following:
So this does not actually test the translated node shows up on a translated page because a translated page is never requested. Also the translation has no difference from the base content ($edit is the same). Is this just to ensure that it will not blow up? It looks to me like it would be very simple to make the translation a bit different in $edit and test that requesting the translated page shows the right translation of the node.
Is this not used anymore anywhere? Was this not used for the front page / main RSS feed? Is there a replacement for that?
The comment does not seem to match the code. Also it looks odd this needs to be done? Why?
Comment #174
dawehnerGood catch, let's translate the value, but also ensure that we use random string rather than a random machine name.
Now this though fails, because we don't have the language filter enabled by default, so we are blocked here on the views data issue as well.
node_feed()
used to be used for/rss.xml
but as that is a view, it was just "abused" for taxonomy/term/feedI don't think we need to provide a similar API as node_feed, especially because zend feed component is actually way more superior already.
Is the new documentation better?
Comment #175
Gábor Hojtsy@dawehner: Changes look great. For taxonomy language filtering, my understanding from #2320743: Taxonomy views needs filter/field on original language is that taxonomy does have a translation language filter, which can be used. It only lacks an original language filter, which we don't want to use here, right?
Comment #176
dawehnerPlease take in mind that you actually still looking at nodes on
taxonomy/{taxonomy_term}
and only nodes (unrelated follow up issue: #2334011: Document that taxonomy/term is just aware of nodes) so the needed language filter here is indeed the one for nodes, which itself is postponed, on the views data issue.So well indeed we don't want to use the language filter for taxonomy here.
Comment #177
Gábor HojtsyRight, its for nodes. The front page and the node admin views already have the language filter we would use here then. Also node provided views blocks almost have them (#2323899: Provided default Node views need language filtering). So I don't believe we need to postpone this at all?
Comment #178
dawehnerOkay let's do it, this issue is critical enough already.
Comment #179
Gábor HojtsyYay, looks great! Thanks so much for working this out :)
Comment #180
dawehnerHere is what you could have reviewed.
Are we sure that this is right? I don't think so. The ID of the view should be entity.taxonomy_term.canonical
This should be documented why we need to come such late.
Comment #181
Gábor HojtsyComment #182
catchAlso could someone post before/after with the query again, ideally with an EXPLAIN? Doesn't look like that happened since #128.
Comment #183
xjmNot sure about #180 but I'll get the EXPLAINs.
Comment #184
xjmPatch needed a reroll; didn't incorporate #180.
Comment #186
xjmQueries before conversion
Query after conversion
Edit: Pasted wrong query :)
Comment #191
xjmUpdated https://www.drupal.org/node/2084727 to remove the reference to the depth argument (which is not a part of D7 core).
Comment #192
xjmAside, taxonomy_term_load_parents(), which is called both in HEAD and with the patch before the view is built is doing a filesort. But the patch doesn't touch that part of the page build.
Comment #193
xjmComment #194
dawehnerRule 1: don't trust yourself
{}_field_data
if available.Comment #195
tim.plunkettWowww. @dawehner++
Comment #196
jibranThis test is missing from current patch.
Comment #197
jibranRe-add the test.
Comment #198
xjmThe query after #1740492: Implement a default entity views data handler looks the same:
SELECT taxonomy_index.sticky AS taxonomy_index_sticky, taxonomy_index.created AS taxonomy_index_created, node.nid AS nid, node_field_data.langcode AS node_field_data_langcode FROM node node INNER JOIN taxonomy_index taxonomy_index ON node.nid = taxonomy_index.nid INNER JOIN node_field_data node_field_data ON node.nid = node_field_data.nid WHERE (( (taxonomy_index.tid = :taxonomy_index_tid) )) ORDER BY taxonomy_index_sticky DESC, taxonomy_index_created DESC LIMIT 11 OFFSET 0
@dawehner, is there a separate Views followup to rely only on the field data table?
Comment #199
xjmThat's my fault; I rerolled #174 instead of #178 I think. Nice catch @jibran!
Attached reincorporates all of #178. Interdiff is against @dawehner's patch from #194 (so includes @jibran's partial fix).
Comment #200
xjmCrossposting with myself... good times.
Comment #202
jibranIf status is true then I think we can remove these view enable lines.
Comment #203
dawehnerjibran++++++ seriously good catch!
Nope, not yet.
Comment #204
jibranWe can remove this as well ;)
Comment #206
dawehnerFixed the merge conflict.
Comment #207
xjmNice, this is looking good. Re-TBC I think? @dawehner addressed all @dawehner's feedback ;) and answers re: queries for @catch's review are in the summary.
Comment #208
catchWith the follow-up to clean up that query it's OK, indexes are fine at least. Thanks for posting the explain and linking to the follow-ups.
Couple of things though:
'classificatio'
In general the help text is quite clunky here but that seems inherited from the existing text.
Also I think this needs its own change notice for site builders about the removal of the RSS feed - that's potentially a lot of broken links.
Comment #209
Gábor Hojtsy@catch: there is no feed lost here. The patch removes this route:
But the view exposes a feed display at
path: taxonomy/term/%/feed
(so same path). It does seem to be true that this removed line has no equivalent in the new code:HOWEVER #2251121: Support to add a feed icon on pages with parameters was mentioned above as a followup required to do that. Views has the capability to assign the feed to the page display, so it would show the feed, but not if the view has arguments. So it does not seem like a separate change notice is in order at all? Also, there is already https://www.drupal.org/node/2084727 as change notice assigned for site builders, which makes best sense to me.
So looks like this is down to the "classificatio" typo fix. Hand fixed that in this patch.
Comment #210
xjmComment #211
xjmYeah the feed itself is still there and now provided by a display of the view; it's just that the icon linking to it is missing. (The icon is a broken link in HEAD.)
The old bit about the changing feed path was reverted when we took out the depth filter.
I removed stale references to the depth filter in the CR and added the bit about the RSS feed icon to the summary; I'd forgotten about that.
Comment #212
xjmComment #213
webchickLooks like catch has already been all over this one, so assigning.
Comment #214
xjmComment #215
jibranAfter #2336177: ConfigurableLanguage cannot be created/removed directly, integrate language_save() and language_delete() this is changed to
ConfigurableLanguage::createFromLangcode('ur');
so it just needs a quick reroll.Comment #216
dawehnerThank you for the pointer!
Comment #217
catchThanks! Committed/pushed to 8.0.x
Comment #219
Gábor HojtsyAmazing, thanks a lot all!
Comment #220
dawehnerCool, thank you!
Comment #221
ParisLiakos CreditAttribution: ParisLiakos commentedYay! thanks everyone for finishing this one!
Comment #222
jibranFinally. We have to update some change notices.
Thank you for committing this almost everyone worked on this issue :)
Comment #224
larowlanRelated follow-up
#2888756: Crawl errors in forum page because of feed url
Forum was unconditionally adding a link to the RSS feed to the page HEAD (as well as doing it wrong, but that's another story).
But the feed is only available if views is enabled (the route was removed here).
Reviews welcome.