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

Refactor for MODX 3 (for a separate branch to the current 2.x) #156

Merged
merged 14 commits into from
Mar 26, 2022

Conversation

muzzwood
Copy link
Contributor

It's in a working state after the refactor. Still a work in progress though. Some of the update/notification services may not yet be working.

I also need to redo the build script and get a resolver together to update the resource class_key fields in the database.

@muzzwood
Copy link
Contributor Author

The following PRs for core and Archivist need to be made for this to work properly too:

modxcms/revolution#16079
modxcms/Archivist#17

@JoshuaLuckers
Copy link
Contributor

@muzzwood thanks for helping out! Do you want me to create a separate branch for these changes?
We could also submit a request to MODX LLC to grant you maintainer access to this repo if that's more convenient.

@muzzwood
Copy link
Contributor Author

Oh a separate branch would be fantastic, thanks! :)

@JoshuaLuckers JoshuaLuckers changed the base branch from develop to develop-3x February 25, 2022 12:11
@JoshuaLuckers
Copy link
Contributor

@muzzwood done

@muzzwood
Copy link
Contributor Author

I plan on testing it some more but so far it seems to be working nicely.
It could certainly use a bit of CSS polish to match the MODX 3 theme though.

Some pics:

Screenshot 2022-02-27 at 17-12-46 Editing Articles MODX Revolution

Screenshot from 2022-02-27 17-13-52

Screenshot from 2022-02-27 17-13-35

@muzzwood muzzwood marked this pull request as ready for review February 27, 2022 09:21
@rthrash
Copy link
Member

rthrash commented Feb 28, 2022

This pull request has been mentioned on MODX Community. There might be relevant details there:

https://community.modx.com/t/upgrademodx-working-to-upgrade-to-modx-3/5017/2

@JoshuaLuckers
Copy link
Contributor

@muzzwood amazing work! I'll try my best to test it sometime in the upcoming days.

@JoshuaLuckers
Copy link
Contributor

@muzzwood any idea why this is happening?
Screen Shot 2022-03-01 at 13 27 47

@SnowCreative
Copy link

Is Quip MODX 3 compatible?

@muzzwood
Copy link
Contributor Author

muzzwood commented Mar 2, 2022

Huh. Not sure what's going on there - some form of escaping perhaps? I know Articles hard-codes a bunch of snippet calls into its templates. I'll look into that tomorrow.

@muzzwood
Copy link
Contributor Author

muzzwood commented Mar 4, 2022

This is an odd one... Any uncached snippet tags added to the template won't be parsed.

@JoshuaLuckers
Copy link
Contributor

@muzzwood The template refers to placeholders like comments.

The values of these placeholders are set here:

$this->getCommentsCall($settings);
$this->getCommentsReplyCall($settings);
$this->getCommentsCountCall($settings);

I think the parser is doing something odd.

@muzzwood
Copy link
Contributor Author

Yeah it's interfering somewhere with the parser but I haven't worked out where yet. 😅

It affects any uncached snippet at all. You can create a new snippet, add it to a template that Articles is using, and you'll see it there unparsed.

@JoshuaLuckers
Copy link
Contributor

You can create a new snippet, add it to a template that Articles is using, and you'll see it there unparsed.

If you create a new resource (document) with template sample.ArticleTemplate the uncached snippet works fine.

So my guess is that, since a regular resource parses it just fine and Article is an extension of this class, it has something to do with the getContent method in the Article class:

$content = parent::getContent($options);

@muzzwood
Copy link
Contributor Author

I've spent a couple of hours this afternoon trying to solve it but no luck.
I've disabled comments by default for now. (They can still be turned on in the Advanced Settings tab on the Articles container).

While comments are disabled, the issue doesn't show itself and all other snippets work properly.

I've tried the same Quip calls on a non-Articles resource/template and they work fine. So it's not Quip by itself that has the problem I think.

@JoshuaLuckers
Copy link
Contributor

@muzzwood I found the solution, some of the placeholders in the templates should be uncached, e.g

[[!+comments_enabled:is=`1`:then=`&nbsp;| <a href="[[~[[*id]]]]#comments" class="comments">Comments ([[!+comments_count]])</a>`]]

@muzzwood
Copy link
Contributor Author

muzzwood commented Mar 26, 2022

Well done @JoshuaLuckers! I can confirm that works - I just pushed the change.

I'm still not entirely sure why that's happening though... 🤔. I would have thought the values would just end up cached rather than the tags themselves not being parsed.

I think this is at a good enough stage for an alpha v3 release. Quip and Archivist probably still need some tweaks though.
Perhaps in the next release, we can rework the resource template to look a bit nicer with the MODX 3.x layout.

@JoshuaLuckers
Copy link
Contributor

I really appreciate you helping out! I think we should add you as a sponsor of this update in the release notes!

@muzzwood
Copy link
Contributor Author

muzzwood commented Mar 26, 2022

Hey, my pleasure! Thanks for all you do too, Joshua. 😃
It was Mark who nudged me to help out, so if it's ok with you, reckon we could put modmore as the sponsor?

@JoshuaLuckers
Copy link
Contributor

Sounds good to me!

@JoshuaLuckers JoshuaLuckers merged commit b7a456b into modxcms:develop-3x Mar 26, 2022
@rthrash
Copy link
Member

rthrash commented Apr 1, 2022

This pull request has been mentioned on MODX Community. There might be relevant details there:

https://community.modx.com/t/modx3-after-update-all-resources-in-web-vanished-suspected-issues-with-articles/5141/2

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

Successfully merging this pull request may close these issues.

4 participants