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

ENH: Localisation manager links available for CMS edit link capable models. #838

Merged
merged 2 commits into from
Apr 8, 2024

Conversation

mfendeksilverstripe
Copy link
Contributor

@mfendeksilverstripe mfendeksilverstripe commented Apr 4, 2024

Description

Allow all CMS edit link capable models to use a Title link in the Locale manager. This link is quite handy as you can switch between locales easily. Previously this was available only for models extending SiteTree (pages).

Screenshot 2024-04-05 at 11 35 15 AM

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

@GuySartorelli
Copy link
Contributor

Please change the commit prefix to FIX as per the commit message guidelines - I've unticked that box in the checklist, feel free to tick it again when you've made this change.

As this is a bug fix that doesn't introduce new API, please also retarget this to the 7.0 branch so it can be released as a patch. I've unticked the relevant checkbox for this in the checklist as well.

Note you may need to reset your commits after retargetting the PR.

@mfendeksilverstripe mfendeksilverstripe force-pushed the bugfix/cms-links branch 3 times, most recently from 69ae287 to ab28c21 Compare April 4, 2024 23:34
@mfendeksilverstripe mfendeksilverstripe changed the base branch from 7 to 7.0 April 4, 2024 23:34
@mfendeksilverstripe
Copy link
Contributor Author

I've pushed up the requested changes @GuySartorelli , please review.

@GuySartorelli
Copy link
Contributor

GuySartorelli commented Apr 4, 2024

There are failing unit tests to address.
It looks like you changed the base after pushing the commit, so you may need to force push your changes to get CI to rerun against the correct parent branch.

@mfendeksilverstripe mfendeksilverstripe force-pushed the bugfix/cms-links branch 2 times, most recently from 863321a to 83e5d76 Compare April 4, 2024 23:53
@mfendeksilverstripe
Copy link
Contributor Author

Tests passing now @GuySartorelli , please review 🙏 .

Copy link

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

Not a full review, just a few things that were jumping at me when I glance at it.

src/Extension/Traits/FluentObjectTrait.php Outdated Show resolved Hide resolved
src/Extension/Traits/FluentObjectTrait.php Outdated Show resolved Hide resolved
$localeTitle = Convert::raw2xml($recordLocale->getTitle());
$render = sprintf('<a href="%s" target="_top">%s</a>', $localeLink, $localeTitle);

return $render;
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried this with return DBField::create_field('HTMLVarchar', $render); and that works for me locally.
When I checked out the 7.0 branch I found that that works for me as well.

Did you replicate the original issue in a fresh installation?

Copy link
Contributor Author

@mfendeksilverstripe mfendeksilverstripe Apr 5, 2024

Choose a reason for hiding this comment

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

yup, tested this with vanilla install with this composer file (ran composer update just before the test): (replaced fluent folder with the one from my branch though)

{
    "name": "silverstripe/installer",
    "type": "silverstripe-recipe",
    "description": "The SilverStripe Framework Installer",
    "require": {
        "php": "^8",
        "silverstripe/recipe-plugin": "^2",
        "silverstripe/recipe-cms": "~5.x-dev",
        "silverstripe-themes/simple": "~3.2.0",
        "silverstripe/login-forms": "^5",
        "dnadesign/silverstripe-elemental": "^5",
        "maxime-rainville/anyfield": "^0.0.0",
        "silverstripe/tagfield": "^3.1",
        "silverstripe-terraformers/gridfield-rich-filter-header": "dev-master",
        "tractorcow/silverstripe-fluent": "^7"
    },
    "require-dev": {
        "silverstripe/recipe-testing": "^3",
        "silverstripe/graphql-devtools": "^1.0"
    },
    "extra": {
        "resources-dir": "_resources",
        "project-files-installed": [
            "app/.htaccess",
            "app/_config.php",
            "app/_config/mimevalidator.yml",
            "app/_config/mysite.yml",
            "app/src/Page.php",
            "app/src/PageController.php",
            "behat.yml",
            "phpcs.xml.dist",
            "phpunit.xml.dist"
        ],
        "public-files-installed": [
            ".htaccess",
            "index.php",
            "web.config"
        ]
    },
    "repositories": {
        "ichaber/silverstripe-swiftype": {
            "type": "git",
            "url": "https://github.com/silverstripe-terraformers/silverstripe-swiftype.git"
        }
    },
    "config": {
        "process-timeout": 600,
        "allow-plugins": {
            "composer/installers": true,
            "silverstripe/vendor-plugin": true,
            "silverstripe/recipe-plugin": true
        }
    },
    "prefer-stable": true,
    "minimum-stability": "dev"
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It's very important to note that that is not a vanilla installation - there are a lot of optional modules there which you won't get when you just do composer create-project silverstripe/installer && composer require tractorcow/silverstripe-fluent

With this set of modules I do experience the problem, but not when I just do a truly vanilla installation. I suspect one of the additional optional modules in this set is interfering with the way the value is displayed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aha! It's the constraint for silverstripe/recipe-cms here that changes things.

"silverstripe/recipe-cms": "5.1.x-dev" works as expected without this PR
"silverstripe/recipe-cms": "~5.x-dev" does not work as expected without this PR

Looks like there's a regression in framework or somewhere similar, and the problem should be addressed wherever that root cause regression lies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent find @GuySartorelli ❤️ . I will change this PR into enhancement only covering the part that expands the display of title links which was my original intention. I'll check if I can find the source of the regression separately.

@GuySartorelli
Copy link
Contributor

re: #838 (comment)

I'll mark this as a draft in the meantime to avoid it being merged in its current state.

@GuySartorelli GuySartorelli marked this pull request as draft April 7, 2024 21:50
@mfendeksilverstripe mfendeksilverstripe changed the title BUG: Localisation manager links correction. ENH: Localisation manager links available for CMS edit link capable models. Apr 8, 2024
@mfendeksilverstripe mfendeksilverstripe marked this pull request as ready for review April 8, 2024 00:42
@mfendeksilverstripe
Copy link
Contributor Author

This is now ready to be reviewed as a standalone enhancement @GuySartorelli

@GuySartorelli
Copy link
Contributor

Please retarget this to the 7 branch since this is no longer fixing a bug

@mfendeksilverstripe
Copy link
Contributor Author

Created silverstripe/silverstripe-framework#11191 to cover the malformed HTML

@mfendeksilverstripe
Copy link
Contributor Author

@GuySartorelli Updated base branch to 7 as requested. Please review.

@GuySartorelli
Copy link
Contributor

Works well locally, just one suggested change above.

@mfendeksilverstripe
Copy link
Contributor Author

I've added the requested change @GuySartorelli , please review.

Comment on lines +126 to +128
$url = Director::makeRelative($url);

if ($url) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I double checked and Director::makeRelative() returns an empty string if an empty string is passed in, so while this path is slightly different to how it used to be, it should be fine.

Copy link
Contributor

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

LGTM and works as expected locally.
Merge on green

@GuySartorelli GuySartorelli merged commit 55c3681 into 7 Apr 8, 2024
20 checks passed
@GuySartorelli GuySartorelli deleted the bugfix/cms-links branch April 8, 2024 04:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants