🎨 Added a configuration option for the paging parameter name #23553
Conversation
WalkthroughThe changes introduce support for a configurable pagination page parameter throughout the codebase. Instead of hardcoding the page parameter as "page", the code now retrieves its name from a new configuration setting ( 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (10)
⏰ Context from checks skipped due to timeout of 90000ms (9)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ghost/core/test/unit/frontend/services/routing/controllers/collection.test.js (1)
119-147: Test implementation looks good with minor suggestion on cleanup pattern.The test correctly validates the custom page parameter functionality by:
- Stubbing the config to return a custom parameter name ('seite')
- Setting the request parameter using the custom name
- Verifying the controller processes it correctly
However, consider using a more standard cleanup approach:
- this.currentTest.callback = function () { - configStub.restore(); - }; + // Move restore to end of test or use afterEach pattern + return controllers.collection(req, res, failTest(done)).then(function () { + themeEngine.getActive.calledOnce.should.be.true(); + security.string.safe.calledOnce.should.be.false(); + fetchDataStub.calledOnce.should.be.true(); + ownsStub.calledOnce.should.be.true(); + configStub.restore(); // Restore here + done(); + }).catch(done);ghost/core/core/frontend/meta/paginated-url.js (1)
11-11: Consider performance implications of config lookup.The implementation correctly retrieves the pagination parameter from configuration. However, calling
config.get()on every function invocation could impact performance if this function is called frequently.Consider caching the config value or evaluating if the performance trade-off is acceptable for hot-reloading configuration changes.
If config caching is desired:
+let cachedPageParam; +function getPageParam() { + if (!cachedPageParam) { + cachedPageParam = config.get('pagination:pageParameter'); + } + return cachedPageParam; +} function getPaginatedUrl(page, data, absolute) { // If we don't have enough information, return null right away if (!data || !data.relativeUrl || !data.pagination) { return null; } - const pageParam = config.get('pagination:pageParameter'); + const pageParam = getPageParam();ghost/core/test/unit/frontend/meta/paginated-url.test.js (1)
170-214: Well-structured test suite for custom pagination parameter.The new test suite effectively validates the configurable pagination parameter feature:
✅ Strengths:
- Follows established testing patterns with proper setup/teardown using
before/afterhooks- Uses
callThrough()to maintain default behavior for other config keys- Tests both index collection and named collection scenarios
- Assertions correctly verify the custom parameter (
seite) in generated URLs✅ Coverage: The tests adequately cover the core functionality, though they're more focused than the comprehensive coverage in existing test suites.
Minor suggestion for enhanced test coverage:
Consider adding a test case for the second page scenario to match the existing test structure:
+ it('should calculate correct urls for the second page of an index collection', function () { + // Setup tests + data.relativeUrl = '/seite/2/'; + data.pagination = {prev: 1, next: 3}; + + // Execute test + const urls = getTestUrls(); + + // Check results + urls.should.have.property('next', 'http://127.0.0.1:2369/seite/3/'); + urls.should.have.property('prev', 'http://127.0.0.1:2369/'); + urls.should.have.property('page1', '/'); + urls.should.have.property('page5', '/seite/5/'); + urls.should.have.property('page10', '/seite/10/'); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 6759d12 and 9bf5a7e00f11b3d1a220c7ee7672659100610676.
📒 Files selected for processing (10)
ghost/core/core/frontend/meta/paginated-url.js(1 hunks)ghost/core/core/frontend/services/rendering/context.js(2 hunks)ghost/core/core/frontend/services/rendering/templates.js(1 hunks)ghost/core/core/frontend/services/routing/controllers/channel.js(2 hunks)ghost/core/core/frontend/services/routing/controllers/collection.js(2 hunks)ghost/core/core/frontend/services/routing/middleware/page-param.js(2 hunks)ghost/core/core/shared/config/defaults.json(1 hunks)ghost/core/test/unit/frontend/meta/paginated-url.test.js(2 hunks)ghost/core/test/unit/frontend/services/routing/controllers/channel.test.js(2 hunks)ghost/core/test/unit/frontend/services/routing/controllers/collection.test.js(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
ghost/core/core/frontend/services/routing/controllers/collection.js (6)
ghost/core/core/frontend/meta/paginated-url.js (2)
config(3-3)pageParam(11-11)ghost/core/core/frontend/services/rendering/templates.js (2)
config(9-9)pageParam(184-184)ghost/core/core/frontend/services/routing/controllers/channel.js (3)
config(8-8)pageParam(27-27)pathOptions(29-32)ghost/core/core/frontend/services/rendering/context.js (2)
config(22-22)pageParam(25-25)ghost/core/core/frontend/services/routing/middleware/page-param.js (2)
config(4-4)pageParam(20-20)ghost/core/test/unit/frontend/services/routing/controllers/collection.test.js (2)
config(11-11)req(21-21)
ghost/core/core/frontend/meta/paginated-url.js (6)
ghost/core/core/frontend/services/rendering/templates.js (2)
config(9-9)pageParam(184-184)ghost/core/core/frontend/services/routing/controllers/channel.js (2)
config(8-8)pageParam(27-27)ghost/core/core/frontend/services/rendering/context.js (2)
config(22-22)pageParam(25-25)ghost/core/core/frontend/services/routing/controllers/collection.js (3)
config(10-10)require(6-6)pageParam(26-26)ghost/core/core/frontend/services/routing/middleware/page-param.js (2)
config(4-4)pageParam(20-20)ghost/core/test/unit/frontend/meta/paginated-url.test.js (3)
config(5-5)getPaginatedUrl(2-2)data(8-8)
ghost/core/core/frontend/services/routing/controllers/channel.js (8)
ghost/core/core/frontend/meta/paginated-url.js (2)
config(3-3)pageParam(11-11)ghost/core/core/frontend/services/rendering/templates.js (2)
config(9-9)pageParam(184-184)ghost/core/core/frontend/services/rendering/context.js (2)
config(22-22)pageParam(25-25)ghost/core/core/frontend/services/routing/controllers/collection.js (4)
config(10-10)require(6-6)pageParam(26-26)pathOptions(28-31)ghost/core/core/frontend/services/routing/middleware/page-param.js (2)
config(4-4)pageParam(20-20)ghost/core/test/unit/frontend/services/routing/controllers/channel.test.js (2)
config(10-10)req(20-20)ghost/core/test/unit/frontend/meta/paginated-url.test.js (1)
config(5-5)ghost/core/test/unit/frontend/services/routing/controllers/collection.test.js (2)
config(11-11)req(21-21)
ghost/core/test/unit/frontend/meta/paginated-url.test.js (3)
ghost/core/test/unit/frontend/services/routing/controllers/channel.test.js (3)
sinon(3-3)config(10-10)configStub(111-111)ghost/core/test/unit/frontend/services/routing/controllers/collection.test.js (3)
sinon(3-3)config(11-11)configStub(120-120)ghost/core/core/frontend/meta/paginated-url.js (1)
config(3-3)
ghost/core/core/frontend/services/rendering/context.js (5)
ghost/core/core/frontend/meta/paginated-url.js (2)
config(3-3)pageParam(11-11)ghost/core/core/frontend/services/rendering/templates.js (2)
config(9-9)pageParam(184-184)ghost/core/core/frontend/services/routing/controllers/channel.js (2)
config(8-8)pageParam(27-27)ghost/core/core/frontend/services/routing/controllers/collection.js (3)
config(10-10)require(6-6)pageParam(26-26)ghost/core/core/frontend/services/routing/middleware/page-param.js (2)
config(4-4)pageParam(20-20)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 22.13.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Lint
🔇 Additional comments (22)
ghost/core/core/shared/config/defaults.json (1)
276-278: LGTM! Clean configuration addition.The new pagination configuration follows existing patterns and maintains backward compatibility by defaulting to "page". This provides a solid foundation for the configurable pagination parameter feature.
ghost/core/core/frontend/services/rendering/templates.js (2)
184-184: Good implementation of configurable page parameter.The dynamic retrieval of the page parameter name from configuration enables the customization feature while maintaining clean code structure.
189-189: Correct dynamic parameter access.Using
req.params[pageParam]instead of the hardcodedreq.params.pageproperly implements the configurable pagination parameter feature.ghost/core/core/frontend/services/routing/controllers/collection.js (3)
10-10: Correct config import for dynamic pagination parameter.The import enables access to the configurable pagination parameter setting.
26-26: Good implementation of dynamic page parameter retrieval.Fetching the page parameter name from configuration allows for the localization use case described in the PR objectives.
29-29: Correct dynamic parameter access implementation.Using
req.params[pageParam]properly implements the configurable pagination feature while maintaining the same logic flow.ghost/core/core/frontend/services/routing/controllers/channel.js (2)
27-27: Good implementation of dynamic page parameter retrieval.The logic correctly retrieves the configurable page parameter name from configuration.
30-30: Correct dynamic parameter access implementation.Using
req.params[pageParam]properly implements the configurable pagination feature while maintaining existing logic.ghost/core/test/unit/frontend/services/routing/controllers/collection.test.js (1)
11-11: LGTM: Configuration import added for pagination parameter testing.The config module import is correctly added to support testing the new configurable pagination parameter functionality.
ghost/core/core/frontend/meta/paginated-url.js (3)
3-3: LGTM: Configuration module imported for pagination parameter.The config import is correctly added to support the configurable pagination parameter feature.
14-14: LGTM: URL path construction updated to use configurable parameter.The hardcoded '/page/' path is correctly replaced with the dynamic pageParam, maintaining the same URL structure while allowing customization.
18-18: LGTM: Regex pattern updated to use configurable parameter.The regex pattern correctly uses the dynamic pageParam instead of hardcoded 'page', ensuring URL matching works with custom pagination parameters.
ghost/core/test/unit/frontend/services/routing/controllers/channel.test.js (2)
10-10: LGTM: Configuration import added for pagination parameter testing.The config module import is correctly added to support testing the configurable pagination parameter functionality, consistent with the collection controller test.
110-137: Test implementation is consistent and correct.The test properly validates the custom page parameter functionality for the channel controller, following the same pattern as the collection controller test. The implementation correctly:
- Stubs the config to return 'seite' as the custom parameter
- Sets the request parameter using the custom name
- Verifies the controller processes it correctly
The same cleanup pattern consideration applies here as mentioned in the collection test review.
ghost/core/core/frontend/services/rendering/context.js (3)
22-22: LGTM: Configuration import added for pagination parameter.The config module import is correctly added to support the configurable pagination parameter feature.
25-27: Excellent implementation of configurable pagination parameter.The changes correctly transform the hardcoded pagination parameter handling into a configurable system:
pageParamnow gets the parameter name from configurationpageParamValueextracts the actual page number using the configurable parameter name- Default value of 1 is maintained when the parameter is not present
The logic is sound and maintains backward compatibility while enabling customization.
39-39: LGTM: Context condition updated to use page parameter value.The paged context detection correctly uses
pageParamValue(the actual page number) instead ofpageParam(the parameter name), which is the correct logic for determining if we're on a paged view.ghost/core/test/unit/frontend/meta/paginated-url.test.js (1)
4-5: LGTM! Proper test dependencies added.The addition of
sinonandconfigimports is appropriate for the new test suite that needs to stub configuration values. This follows the same pattern used in other test files across the codebase.ghost/core/core/frontend/services/routing/middleware/page-param.js (4)
4-4: LGTM! Necessary dependency added.The
configimport is correctly added to access the configurable pagination parameter.
12-12: Good documentation update.The comment accurately reflects that the page parameter name is now configurable, improving code clarity.
20-22: Clean implementation of configurable page parameter.The implementation correctly:
- Retrieves the page parameter name from configuration
- Constructs the regex dynamically using the configured parameter
- Maintains the existing URL pattern matching logic
The regex construction is safe since the page parameter comes from configuration and not user input.
34-34: Correct dynamic parameter assignment.Using the configurable parameter name for
req.paramsassignment ensures consistency throughout the request lifecycle. This maintains compatibility with downstream code that expects the parameter to be available under the configured name.
ghost/core/core/frontend/services/routing/controllers/channel.js
Outdated
Show resolved
Hide resolved
SebastianSchroeder
force-pushed
the
custom_page_parameter
branch
4 times, most recently
from
ff357a8 to
e13459c
Compare
|
@cathysarisky I didn't tag this with the i18n emoji but i18n is actually the reason why I opened this PR. What do you say? |
Enables websites to be configured with a custom paging parameter name. This is especially helpful for internationalized installations where it should be a localized parameter (e.g., seite for German) instead of the standard English page parameter.
SebastianSchroeder
force-pushed
the
custom_page_parameter
branch
from
e13459c to
3fe283b
Compare
I think the PR is a great idea, but I'm not going to tag it as i18n, because if I do, there's a good chance that stalebot will close it before someone has time to review it. |
I created this PR as I want to use Ghost for my German website and the option to configure the name of the paging parameter was missing. Currently I have to live with www.geist.de/page/1 when I would rather use www.geist.de/seite/1 as that matches the language of my site.