Fix metadata intrasite links #2288
Conversation
|
Ah, just noticed I probably could have pushed to the previous branch with |
justinmayer
changed the title
|
Hehe. No worries. I was just about to let you know that you could have pushed to the previous branch/PR, but it seems you noticed that on your own. Thanks again for the contribution! |
| if 'summary' in metadata: | ||
| self._summary = metadata['summary'] | ||
| if len(self._context.get('filenames', [])) > 0: | ||
| self.refresh_metadata_intersite_links() |
There was a problem hiding this comment.
@charlesfleche @justinmayer As I commented in #2162 (comment), this line causes link replacement to be dependent on the order in which files are processed -- because it's done during parsing of a single article/page and not later when everything is ready (so e.g. links to pages in metadata fields that were not yet processed are reported as Unable to find).
I investigated further and realized refresh_metadata_intersite_links() is called again later below, after all articles/pages are processed -- and in that second step the links that were previously Unable to find are then replaced correctly. I don't know the code very well, but in my case when I commented the above two lines out, the warnings were gone (and links in metadata fields still correctly replaced). I imagine removing the above might cause some plugin to cease working (since e.g. the summary metadata field won't be present yet), so not really sure how to fix that. On the other hand, I don't think calling refresh_metadata_intersite_links() twice on everything is a good idea (at least from a perf PoV, but I could imagine some potential issues with {attach} as well).
Any ideas how to fix this?
There was a problem hiding this comment.
Agreed, this is not ideal.
Removing these won't make summary vanish, but it'll be unprocessed if one accesses before everything is processed. And that is actually fine, since accessing .summary in a plugin earlier than all_generators_finalized is a "bug" in itself for this same reason (it'll spew Unable to find warnings).
Another issue is that, this removed _summary. Even though it's "internal", some plugins used that.
From what I can see, this modification is all for preventing summary being processed twice.
Edit: Everything is processed twice anyway. I don't know what this is for.
I'd say, reverting this bit and get_summary should be enough.
And maybe excluding summary from refresh_metadata_intersite_links, if one is worried enough about the double processing.
There was a problem hiding this comment.
Finally got back to this (sorry for the delay), the proposed change is in #2406.
And maybe excluding summary from refresh_metadata_intersite_links, if one is worried enough about the double processing.
Not sure what you mean, reverting the above bit is (as far as I can see) enough to stop the double processing.
There was a problem hiding this comment.
I meant, if you revert the above part and get_summary then summary will be processed twice.
- Once when
refresh_metadata_intersite_linksis called - Once when
content.summaryis accessed (or any other way.get_summaryis called).
Furthermore, reverting only this part is pointless, since you end up with a ._summary attribute that's not used anywhere.
There was a problem hiding this comment.
Um, I'm afraid I lost all the context over the time we're discussing this 🙈
Furthermore, reverting only this part is pointless, since you end up with a ._summary attribute that's not used anywhere.
I did that only to fix this:
Another issue is that, this removed
_summary. Even though it's "internal", some plugins used that.
So, what I ended up doing in #2406 is:
- keeping
_summary, because some plugins used that, as you said - not calling
refresh_metadata_intersite_linkstoo soon
I could also just delete this part altogether (and thus _summary would be no longer). Can we move the discussion to #2406?
There was a problem hiding this comment.
Sure, but _summary is not used in get_summary anymore. This PR also changes that. I mean those should be reverted together.
Plugins modifying _summary do so in order to change .summary, since you can't directly access .summary too early. Now they will access and change something irrelevant. Modifying _summary will leave .summary unchanged.
There was a problem hiding this comment.
Oh! Now I get you. I thought _summary was accessed only for read, never for write. Now it all makes sense, sorry for my denseness.
#2406 is updated (the CI is failing for an unrelated reason, same on master), but instead of just reverting the second part I realized the changes to _summary won't be picked up by refresh_metadata_intersite_links() and the _summary handling would need to creep there as well. So now I introduced a _summary property getter/setter that's basically just a "view" onto metadata["summary"]. Also, "because some plugins used that" I assume it's not a workflow worth preserving so the getter/setter now prints a deprecation warning when the property is used.
I hope this is finally getting closer to how it should be :) And sorry for the delays, I'm extremely busy.
|
Also... Did anyone test this metadata intrasite links with That's practically the whole reason for these 'link-containing' fields ( |
|
You mean because now it's processed only once (and exactly once) while before it was processed on each access and thus being able to generate different values based on where the content/summary is accessed from? (Disclaimer: I personally only tried enabling |
|
Exactly. PS: If you're getting invalid links with |
Squashed version of previous PR #2226