Needs work
Project:
Pathologic
Version:
7.x-3.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
4 Aug 2015 at 14:10 UTC
Updated:
17 Aug 2015 at 20:13 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
fietserwinComment #3
fietserwinNow with correct line endings.
Comment #5
fietserwinOK, I have been running the tests locally to see what wen wrong and came up with:
- a test that shows the error (test-only patch should fail).
- a patch that even further cleans up the code (patch should succeed).
Comment #8
fietserwinEditing patches manually is dangerous :(
Comment #9
fietserwinThis is weird: the test really fails locally:
But apparently it is impossible or difficult to reliably fake being running on a sub directory??? The test locally showed /dev/dev/foo (via check_markup) versus /dev/foo (via _pathologic_content_url) and thus did show the error.
To explain the patch:
The code contains this "hellish" if statement that prevents adding the current local path to $filter_settings['local_paths_exploded'] (if it is defined in the "also considered local" setting) only to add it after that enormous if statement. But then why preventing adding it in the first place, and add it at the end, thereby disturbing the order? So the patch does not check anymore if it is the current local path but still does add it at the end, just to be sure it is there. This means it may be there twice, but that doesn't matter. It is subsequently only used to linearly search through that array to find the first match. So a value being there twice in that array is not bad.
Comment #10
Garrett Albright commentedYou're using .= instead of = here.
Anyway, what happens if you just remove the "/" from "also considered local?" I don't see how that would be correct configuration for you if your site is in a subdirectory.
Comment #11
fietserwin- You are right about the .=, fixed that one.
- I'm moving the content between live (/) and (/dev/), and this content can be added/edited on both, thus the content ends up having /dev/path/to/file and /path/to/file links. So for the dev instance it would be correct to define that paths may also start with just a /. and thus that /path/to/file should be changed into /dev/path/to/file.
If I would only define the other path that is *also* considered to be local in the settings, I could not easily move the full database between these 2 sites, but it owuld also lead to the following errors (current code):
Situation 1:
local path: /
also local: /dev/
results:
- /path/to/file => /path/to/file (correct)
- /dev/path/to/file => /path/to/file (correct)
Situation 2:
local path: /dev/
also local: /
results:
- /path/to/file => /dev/path/to/file (correct)
- /dev/path/to/file => /dev/dev/path/to/file (incorrect)
Situation 3:
local path: /
also local: /dev/; /
results:
- /path/to/file => /path/to/file (correct)
- /dev/path/to/file => /path/to/file (correct)
Situation 4:
local path: /dev/
also local: /dev/; /
results:
- /path/to/file => /dev/path/to/file (correct)
- /dev/path/to/file => /dev/dev/path/to/file (incorrect)
This makes me realize that the old patch does still not handle situation 2, so the ordering should be done after making sure the local path has been added as well. New patch does so.
Comment #13
fietserwinI don't get it. These tests pass locally while they fail on a test server, whereas the test only patch (from #8) failed locally (as expected) but passed remotely??? It seems like settings from the host environment leak through and thus the tests are not isolated enough to repeat reliably. Any idea what that could be?
I adapted the tests a bit to emphasize that with the latest patch, the order in the also considered local text area is no longer important. I also changed my test a bit to see if that was why the test-only patch succeeded remotely. Let's see which tests fail this time...
Side note: test messages should not be translated: https://www.drupal.org/node/265828: do not place an additional burden on translators on localize.drupal.org.
Comment #14
fietserwin...
Comment #17
fietserwinLooks like I managed to make my point clear with a test. Now, we have to find out why the 2 other tests are failing. I will try to have a
look at that later this week and probably will have to upload a number of tests with more info just to find out about the values that do not compare.