Custom module fixes #2145
Custom module fixes #2145
Conversation
| @@ -32,6 +32,12 @@ public function entity() | |||
| $publicId = $this->$field; | |||
| } | |||
| } | |||
| if (! $publicId) { | |||
There was a problem hiding this comment.
Can you explain why this change is needed?
There was a problem hiding this comment.
It's to handle the situation for custom modules where the entityType is different from the pluralized version.
app/Providers/AppServiceProvider.php
Outdated
| @@ -125,8 +125,22 @@ public function boot() | |||
| return ''; | |||
| } | |||
|
|
|||
|
|
|||
| if(! Utils::isNinjaProd() && \App::isLocale('en') == false) { | |||
There was a problem hiding this comment.
Handles fixing a mismatched breadcrumb when the module name is different. The reason it specifically looks for the non-English locales is there is another check at line 142 that handles the English locale using the built-in str_singular function.
That being said, I'm wondering if the issue lies deeper inside the module translation code; I might keep looking a bit at it to see if I can fix the issue more elegantly.
There was a problem hiding this comment.
I'm happy to help try to find a better solution, the purpose of the code is a bit hard to understand.
|
Hi, just wanted to follow up. I'd like to merge the PR but the changes affect code paths used by most requests, I want to make sure I fully understand it. |
|
Totally agree. When I looked at it again it seemed hacky and likely to
cause headaches down the road.
---
Cheers,
Chris
Christopher Di Carlo
_Owner_
DI CARLO SYSTEMS INC.
p:
519.291.5656 m: 519.574.5652
a:
86713 Maple Keys Line, Listowel, ON N4W 3G6
w:
www.dicarlosystems.ca [3] e: chris@dicarlosystems.ca
[4] [5] [6]
On 2018-06-11 03:08, Hillel Coren wrote:
@hillelcoren commented on this pull request.
-------------------------
In app/Providers/AppServiceProvider.php [1]:
> @@ -125,8 +125,22 @@ public function boot()
return '';
}
+
+ if(! Utils::isNinjaProd() && \App::isLocale('en') == false) {
I'm happy to help try to find a better solution, the purpose of the code is a bit hard to understand.
--
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub [1], or mute the thread [2].
Links:
------
[1]
#2145 (comment)
[2]
https://github.com/notifications/unsubscribe-auth/ADUm6BbKnLIXyLZxktD9qtSSCGcIc9e7ks5t7hd2gaJpZM4UZnNH
[3] http://www.dicarlosystems.ca
[4] https://facebook.com/dicarlosystems
[5] https://twitter.com/dicarlosystems
[6] https://www.linkedin.com/in/christopher-di-carlo-214bb321/
|
|
If you can improve it it'd be great, otherwise if you can separate out the change I'll merge the rest. |
chrisdicarlo
force-pushed
the
custom-module-fixes
branch
from
1fa51a5
to
07da5e6
Compare
|
I modified the AppServiceProvider to check the breadcrumb against a defined 'base-route' entry in the module.json. This way if the module defines the base-route it will correctly resolve the module name for further translation but if not, it will just fallback to the currently defined behaviour. This leverages previous changes added in commit 035ac82. |
Some fixes for custom modules to handle non-default routes, etc