Currently, you can go to admin/structure/feeds/i_dont_exist, and Feeds will show an edit form for a new importer, this is just weird, and we should return a 404.

CommentFileSizeAuthor
#10 interdiff.txt1.76 KBtwistor
#6 2305919-non-existent.png83.87 KBmegachriz
#6 2305919-FeedsHTTPFetcher.png106.38 KBmegachriz
#6 2305919-log.png39.02 KBmegachriz
#4 interdiff.txt598 bytestwistor
#4 feeds-importer-404-2305919-4.patch4.61 KBtwistor
#2 feeds-importer-404-2305919-2.patch4.81 KBtwistor
#1 feeds-importer-404-2305919-1.patch395 bytestwistor

Comments

twistor’s picture

twistor commented
Status: Active » Needs review
StatusFileSize
new feeds-importer-404-2305919-1.patch395 bytes
twistor’s picture

twistor commented
StatusFileSize
new feeds-importer-404-2305919-2.patch4.81 KB

Status: Needs review » Needs work

The last submitted patch, 2: feeds-importer-404-2305919-2.patch, failed testing.

twistor’s picture

twistor commented
Status: Needs work » Needs review
StatusFileSize
new feeds-importer-404-2305919-4.patch4.61 KB
new interdiff.txt598 bytes
megachriz’s picture

megachriz
Primary language Dutch
commented

I believe something weird also happens when entering the url for a existing, but not currently chosen fetcher/parser/processor on a feeds importer that does exist. Hope to look at this patch in a few days.

megachriz’s picture

megachriz
Primary language Dutch
commented
StatusFileSize
new 2305919-log.png39.02 KB
new 2305919-FeedsHTTPFetcher.png106.38 KB
new 2305919-non-existent.png83.87 KB

With patch applied, 404's are correctly returned for:

  • /import/non_existent
  • /import/non_existent/delete-items
  • /import/non_existent/unlock
  • /admin/structure/feeds/non_existent
  • /admin/structure/feeds/non_existent/settings
  • /admin/structure/feeds/non_existent/fetcher
  • /admin/structure/feeds/non_existent/settings/FeedsFileFetcher
  • /admin/structure/feeds/non_existent/parser
  • /admin/structure/feeds/non_existent/settings/FeedsCSVParser
  • /admin/structure/feeds/non_existent/processor
  • /admin/structure/feeds/non_existent/settings/NodeProcessor
  • /admin/structure/feeds/non_existent/mapping

For the following paths no 404's are returned (see also attached images):

  • /import/non_existent/log
    This page is returned by a View. The feeds importer is set as a contextual filter, but there is no feeds importer validator available. Should we implement such one or is that overkill?
  • /admin/structure/feeds/node/settings/FeedsHTTPFetcher (when FeedsFileFetcher is the active fetcher)
    One can go to setting pages of fetchers, parsers and processors, even when these are not selected for the importer. Ideally, the path to the plugin's setting page should be something like /admin/structure/feeds/%feeds_importer/fetcher/settings or would that possibly break contrib (if, for example, a contrib introduced a fourth type [not fetcher, parser or processor] of plugin)?
  • /admin/structure/feeds/node/non_existent
    This results in an empty page instead of a 404.
twistor’s picture

twistor commented
  • /import/non_existent/log
  • /admin/structure/feeds/node/settings/FeedsHTTPFetcher

These should be fixed, but I think we can push them off into separate issues. I agree that the /admin/structure/feeds/node/settings/FeedsHTTPFetcher should be /admin/structure/feeds/node/settings/fetcher, but that's going to be a huge patch, lots of tests and things will change.

  • /admin/structure/feeds/node/non_existent

I'll fix this one up.

megachriz’s picture

megachriz
Primary language Dutch
commented

Sounds good to move "/import/non_existent/log" and "/admin/structure/feeds/node/settings/FeedsHTTPFetcher" to separate issues. They aren't big "showstoppers" anyway.

  • /admin/structure/feeds/node/non_existent

I'll fix this one up.

Great. Be sure to check that it doesn't hurt contrib modules that add extra subpages to the Feeds UI. My sandbox module Feeds Quick Mapping (which provides an alternative way of defining mappings) adds such a subpage.

  • twistor committed edb2384 on 7.x-2.x 
    Issue #2305919 by twistor: Fixed Return 404 when trying to edit a non-...
twistor’s picture

twistor commented
Status: Needs review » Fixed
StatusFileSize
new interdiff.txt1.76 KB

I went ahead and committed this with some minor changes.

Thanks for pointing out Feeds Quick Mapping, which does get broken by the fix for /admin/structure/feeds/node/non_existent.
There's not an easy way at this point to fix that, we'll have to have some sort of API to extend the Feeds UI. But, I want to re-vamp the UI anyway.

twistor’s picture

twistor commented

Followups:
#2333005: Change plugin path from plugin id to plugin type.
#2333009: Add importer validator for Views.
#2333011: Create API for extending the Feeds UI.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.