Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make parsoid transform follow disable_storage setting. #1337

Merged
merged 3 commits into from
Dec 14, 2023

Conversation

brightbyte
Copy link
Contributor

If disable_storage is set, don't try to load the original HTML from cache for transform operations.

Bug: T350219
Change-Id: I33c3baae4f0579121b89f6d913852d6c118ebd56

Copy link
Contributor

@thesocialdev thesocialdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

let contentReq;

if (disabledStorage) {
contentReq = this._getPageBundleFromParsoid(hyper, req);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm that this will fetch a stashed version from core? Otherwise, this could lead to dirty diffs? How are you testing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabling storage also disabeld stashing, since RESTbase uses the storage mechanism to implement the stash. It doesn't use the Stash in MW.

But that should be a non-issue, since VE doesn't use this API anymore. As far as I know, nothing uses stashing in RESTbase. I have a PR up for removing it entirely.

@johngian
Copy link
Collaborator

johngian commented Dec 4, 2023

I think that if this PR gets rebased to current master, CI will work (after fixes on master).

If disable_storage is set, don't try to load the original HTML from
cache for transform operations.

Bug: T350219
Change-Id: I33c3baae4f0579121b89f6d913852d6c118ebd56
Change-Id: Id6f41779decdcd3bd2c7afdc7dbae143b7be5877
@johngian
Copy link
Collaborator

johngian commented Dec 5, 2023

Other than a nit comment, lets squash and merge.

@brightbyte
Copy link
Contributor Author

Other than a nit comment, lets squash and merge.

Which comment?

@johngian johngian merged commit 81218db into wikimedia:master Dec 14, 2023
8 checks passed
Copy link
Contributor Author

@brightbyte brightbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants