search

Lombiq / Open-Source-Orchard-Core-Extensions

Looking for some useful Orchard Core extensions? Here's a bundle solution of all of Lombiq's open-source Orchard Core extensions (modules and themes). Clone and try them out!
https://lombiq.com
BSD 3-Clause "New" or "Revised" License
44 stars 18 forks source link

Upgrade to Orchard Core 1.8 (OSOE-751) #638

Closed Piedone closed 4 months ago

Piedone commented 7 months ago

...once https://github.com/OrchardCMS/OrchardCore/issues/14940 is done.

Jira issue

Piedone commented 7 months ago

@domonkosgabor do you perhaps have something to add from the weekly news?

domonkosgabor commented 7 months ago

@domonkosgabor do you perhaps have something to add from the weekly news?

The documentation of the upcoming release summarizes the breaking changes and other changes so well: https://docs.orchardcore.net/en/latest/docs/releases/1.8.0/

Maybe I can add two minor things:

  1. There are new extensions for IDisplayManager that we can utilize: https://orcharddojo.net/blog/use-shape-when-rendering-http-errors-to-allow-customization-from-the-ui-git-hg-mirror-is-running-on-orchard-core-this-week-in-orchard-01-12-2023
  2. This post mentions the centrally defined media resources. But in the meantime, we have already added other resources to the manifest, so maybe we should check it out and use the named resources: https://orcharddojo.net/blog/introduce-a-new-navbar-shape-stimata-hotel-is-using-orchard-core-this-week-in-orchard-10-11-2023
Piedone commented 7 months ago

Thanks!

Piedone commented 5 months ago

Note this discussion about IAsyncConfigureOptions @Psichorex: https://github.com/OrchardCMS/OrchardCore/pull/15125

Psichorex commented 5 months ago

Update: Onto the next problem We have custom type such as PersonPage ExpressionContent ContentSetType etc. When we navigate to their editor the following devtools exception occures:

And the preview button is broken.

The built in types and types created from Admin are working properly. This $ is not defined is usually due to jQuery not being referenced before a script that uses it is called. However from devtools all I see is jQuery is indeed included in both the problematic types and the built in types. Also this has nothing to do with our newly added Decorator as I disabled the feature and the problem still occured. I am currently trying to find out the cause of this.

sarahelsaig commented 5 months ago

PersonPage only contains PersonPart, whose editor doesn't even use Javascript at all. Same with ExpressionContent, it only contains stock OC content fields.

Psichorex commented 5 months ago

PersonPage only contains PersonPart, whose editor doesn't even use Javascript at all. Same with ExpressionContent, it only contains stock OC content fields.

@sarahelsaig But their preview button uses contentpreview-edit.js or something called like this. I found the error in the meantime. So when these types are considered for some reason this contentpreview-edit.js is being registered prior to jquery.js being registered. Makes no sense to me what happens in the background that produces this.

Even more funny that I had to implement a ResourceManagerDecorator for different reasons and I was able from there to make sure that jQuery is always included first. At least this is a solution to the problem because the exception went away. Apart from this I think this is a bug in the OC 1.8 release but I am really not sure what I can call a bug in OC. It might be a feature.

Well I mean if it's a bug we would have had to make a temp solution for it anyways.

sarahelsaig commented 5 months ago

The preview button and contentpreview.edit.js are part of the OrchardCore.ContentPreview module so that should not be limited to our content items but any content item that can display a preview (may be inconsistent).

The module's resource configuration doesn't declare jQuery as a dependency of the "contentpreview-edit" resource, so it's up to the whims of the resource manager if it gets imported in the right order or not. Nor is a depends-on="jQuery" added to the script tag helper where it's used. This is an OC bug, I suggest opening an issue there.

A quick and dirty workaround is to override the ContentPreview.Button.cshtml with a copy and insert the depends-on attribute. But if you can add jQuery as a dependency to the ResourceDefinition of "contentpreview-edit" in your decorator, that's much better. Either way, please include a comment with a link to the OC issue.

Psichorex commented 5 months ago

https://github.com/OrchardCMS/OrchardCore/issues/15181

Piedone commented 5 months ago

You can temporarily add an override for ContentPreview.Button.cshtml that explicitly requires jQuery to a suitable module until that's fixed.

Psichorex commented 5 months ago

I manually ordered the resources from the decorator so jquery always ends up first. It's more "pleasant" than an override.

Psichorex commented 5 months ago

image

Why did have thetheme-bootstrap-oc in ChartJs samples?

So the following error popped up after fixing the previous: In the OC 1.8.2 branch The UITest TestChartJsSampleBehaviorAsync is broken because ResourceManager thinks that thetheme-bootstrap-oc is a required resource on that page (shown on the above picture) and we don't have that anymore and this is not due to the decorator. FATAL log error comes in as a required resource is not found.

I am asking this to collect data and not solution because I did not work with ChartJS before so I don't know if the required thetheme-bootstrap-oc is blatant in the first place or it is correct.

In OC 1.8.2 the following happens: image image

sarahelsaig commented 5 months ago

Why not search for that "thetheme-bootstrap-oc" string in the code base first? It's here: https://github.com/Lombiq/Orchard-Chart.js/blob/f314e6262328274311a0eb5c9d94008e96cbb349/Lombiq.ChartJs.Samples/Views/_Layout.cshtml#L10

And here is the commit where it originated: https://github.com/Lombiq/Orchard-Chart.js/commit/a0e82a3e610f9f59c4c1cf5471603a7176f87566#diff-8b380456dc41bb9509408f2f227dc615beac595681f2bda8835bc50e249e7b95R9

Looks like the sample just wants an empty page to display the charts in.

Piedone commented 5 months ago

Reordering resource from the decorator is a bad idea, don't do that. The dependency order is complex, and can be determined by a number of factors, you can't just say that jQuery is always at the top.

Psichorex commented 5 months ago

Why not search for that "thetheme-bootstrap-oc" string in the code base first? It's here: https://github.com/Lombiq/Orchard-Chart.js/blob/f314e6262328274311a0eb5c9d94008e96cbb349/Lombiq.ChartJs.Samples/Views/_Layout.cshtml#L10

And here is the commit where it originated: Lombiq/Orchard-Chart.js@a0e82a3#diff-8b380456dc41bb9509408f2f227dc615beac595681f2bda8835bc50e249e7b95R9

Looks like the sample just wants an empty page to display the charts in.

I did a search and nothing popped up. I use visual studio and used ctrl+shift+f and searched the file name. It found nothing.

Reordering resource from the decorator is a bad idea, don't do that. The dependency order is complex, and can be determined by a number of factors, you can't just say that jQuery is always at the top.

I did that because from where it worked Jquery was always in the first places. Also I did remove the error and seemingly nothing else popped up.

Piedone commented 5 months ago

You're testing a tiny fraction of the dependency combinations that are possible in OSOCE, let alone for every solution using our projects. You can't make sure it'll work in every case.

Psichorex commented 5 months ago

image image

Seems like this did not solve the problem. According to ShapeTracing the shape has been overriden but it still fails to find jQuery although it was set as a dependency in the cshtml.

Psichorex commented 5 months ago

You're testing a tiny fraction of the dependency combinations that are possible in OSOCE, let alone for every solution using our projects. You can't make sure it'll work in every case.

I created an OrderBy that is purely bringing jQuery to the first place and keeps every single other item in it's original place. This is what actually happens on the Content Item editors that are working: jQuer is first. image

Psichorex commented 5 months ago

We have a test failing due to HtmlValidation as there is a role="button" in the html code. I found that and it's in the TheTheme theme so I must remove the warning manually.

image

https://github.com/OrchardCMS/OrchardCore/blob/f843b91d479e222474a067573fcb5022814af6ed/src/OrchardCore.Themes/TheTheme/Views/ToggleTheme.cshtml#L2

Piedone commented 5 months ago

Again, no, the dependency order is not so simple. Features can break if you bring jQuery to the top. Don't do that. Include it from a shape override. It'll work for sure and we only need it until the next OC upgrade (do document this there).

Psichorex commented 5 months ago

Again, no, the dependency order is not so simple. Features can break if you bring jQuery to the top. Don't do that. Include it from a shape override. It'll work for sure and we only need it until the next OC upgrade (do document this there).

I did try to use a shape override and also included a comment about that here above https://github.com/Lombiq/Open-Source-Orchard-Core-Extensions/issues/638#issuecomment-1913307502

It doesn't want to use the new ContentPreview_Button shape no matter what I do. All I know is that I should copy the .cshtml to one of our modules that is enabled and reference the original OC module as a dependency. I did that in Lombiq.HE and also from Lombiq.BaseTheme and neither way worked.

From ShapeTracing it was saying that it is coming from HE but the Shape did not include the changes I made to the local shape to make sure it's ours.

Psichorex commented 5 months ago

Well for some reason it works from Lombiq.VueJs

Piedone commented 5 months ago

When is this causing an issue, to begin with? If in UI testing then you can exclude the URL from monkey testing or ignore the error because this is an OC bug.

Psichorex commented 5 months ago

It is causing errors in UITesting on Content Item Edit pages when the Preview button is present but not everytime. It also made an error in a Test that was using TheBlogTheme and it occured there aswell so it is indeed an OC bug. I think we should settle with this shape override now that it finally works because of the multiple places it causes errors.

Piedone commented 5 months ago

Putting it into our Vue module is random though.

Exclude that error from that UI test's assertion then, we have a lot of examples of that.

Psichorex commented 5 months ago

When using primary constructor stringLocalizer T will be problematic due to the analysers.

SO we either still declare a field named T or we remove the rule that warns us to start with lowerCase which is not a good idea imo or we supress.

What do you think?

image

Piedone commented 5 months ago

What's the analyzer violation?

Psichorex commented 5 months ago

That members of the primary constructor should start with a lower-case letter. https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1313.md But I used fields to keep to the IStringLocalizer T convention. So: image This way we don't need supresses and we can keep to the T naming convention. Note this is only needed when we use the stringLocalizer which we usually name T.

Piedone commented 5 months ago

A suppression for this all the time is definitely not good, neither is disabling the parameter naming rule. Please bring this up in Teams for the whole team to be able to chime in, because we'll need a new convention, then; my vote would be on using _t (and _h for IHtmlLocalizer).