[#20229] - Optimization and fix of multilingual associations and add layouts to com_content links
- Fixed in Code Base
- 5 May 2018
- Medium
- Build: staging
- # 20229
- Diff
- Septdir:fix-layouts-and-associations
User tests: Successful: Unsuccessful:
Summary of Changes
This PR contains many fixes based on the following PR's
#19681 #19683 #19314 #20211
Differences from previous PR
- #19681 - Completely cleaned, now the layout is passed to the helper, and not taken from the parameters. That the bug with
?layout=is corrected, as well as allowing developers to output a specific layout as needed - #20211 - Add an additional check on option and view to exclude misunderstandings when an incorrect layout could be passed to the link
- #19314 - Removed an extra query, additionally added checks for publish_up and publish_down, except the conditions themselves now depend on the rights of the user. I was based on the arcitle model
- #19683 - This PR is no longer needed
Testing Instructions
In advance I apologize for the laziness. But I'll just list pr in which you can see Testing Instructions
In the courtyard night. And if all the tests listed in this description it turns out to be too huge
- #20211 -
?layout=bug - #19683 - Remove duplicated queries
- #19681 - Fix link when layout and association (I still think that this is not correct, but I must agree that such a version of associations is the most common)
- #19314 Correcting display of associated articles. Additionally, need to check publish_up/publish_down and also, at the same time check the display of people with editing rights, for example, super user
Explanations about add advanced where clause param (d99dca5)
I thought for a long time how best to do it. It was necessary to take into account an infinite number of options. The highest priority was to ensure that developers do not have to make unnecessary queries to the database.
With this, completely delete what did @infograf768 in #19314 because if the visitor can not go to the article page, then neither the flag nor the link should be displayed.
The obvious option was to have all the necessary conditions passed to libraries/src/Language/Associations.php However, the conditions for a correct selection can be countless.
- Someone uses
statein their components - someone uses
published\trashed - someone who uses
accessdoes not have someone - someone will use
publish_uppublish_down - someone in general needs absolutely different conditions
How many components are there and so many options.
Based on this, I decided to add $advClause param
This is an array through which any developer can add to the requests those where he needs.
Truth had to change the value of $queryKey but according to my understanding $extension, $tablename, $context, $id would be enough to identify the query
P.S I have rather bad English and I often resort to the help of an interpreter, so if something is not clear, or do not properly ask at once, or point out mistakes
@Quy If there are fixes for docblock I will be very grateful if you immediately attach the code
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Administration com_categories Front End com_content Libraries |
| Labels |
Added:
?
|
||
continuous-integration/drone/pr
FILE: ...rc/github.com/joomla/joomla-cms/libraries/src/Language/Associations.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
40 | WARNING | Line exceeds 150 characters; contains 156 characters
--------------------------------------------------------------------------------
UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY
--------------------------------------------------------------------------------
But i can't shorten line by 6 characters. Can I leave 156 characters?
But i can't shorten line by 6 characters. Can I leave 156 characters?
Unfortunately no, because of our existing PHPCS rules you'll have to change the method declaration to this (luckily there's an open task to make this something better):
public static function getAssociations($extension, $tablename, $context, $id, $pk = 'id', $aliasField = 'alias', $catField = 'catid',
$advClause = array())
{
// Code
}(Will look more later, about to sign off for the night and was just quickly clearing unread email queue)
There is no limit to perfection
Looks like working fine here.
We have to make sure it is totally B/C and also see if we need to modify some other core components to use the new variable.
and also see if we need to modify some other core components to use the new variable.
At the moment, all components work as they worked. I checked com_contact, but the additional check is not superfluous
However, in the near future it is desirable to do this like 79ed099 for com_contanct and com_newsfeed
In addition, you will need to add &layout=xxx to links in all templates and modules. This was not initially, and this PR does not affect the work of these extensions, but it still needs to be done.
So after testing, checking and I hope to merge this PR you can create a new issue for each task separately and I or someone else will write the code as far as possible.
It will be more convenient. Because in this PR and so there are 4 Testing Instructions and but they are all related to com_content. So it's relatively convenient to test.
continuous-integration/drone/pr β the build failed JavaScript
2 PR in the last 3 hours - this indicates that people have finally begun to notice the wrong links on sites
I have tested this item
Looks OK here.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20229.
restarted drone
I have tested this item
ΠΠΊ.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20229.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20229.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20229.
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-05-05 20:19:50 |
| Closed_By | ⇒ | mbabker | |
| Labels |
Added:
?
|
||
@Septdir
I am thinking about a possible improvement to this.
Basically, if an associated article is set to registered, we should still be able to display the flag and link to that associated article in frontend when there is a Read More and show_noauth is set to Yes.
I.e. in components/com_content/helpers/association.php
we would not use $advClause[] = 'c2.access IN (' . $groups . ')'; in that case.
When clicking the flag and the item is displayed, a Register to Read More should display.
To test I just commented //$advClause[] = 'c2.access IN (' . $groups . ')'; (without doing the checks).
Here I also have improved the components/com_content/views/article/tmpl/default.php to use the readmore layout. (as it is now, it is not a button)
The issue is the check for params in the target associated article as we do not get its id before looking for associations.
Any idea?
@infograf768 hm..
If one idea however, you might have to make additional query
You can comment
$advClause[] = 'c2.access IN (' . $groups . ')';
Then there will be absolutely all id
But then you have to make a query to get the parameters and the accees value
And then in the cycle do the necessary checks. What in my opinion is not the best idea.
Or finalize getAssociation function
So select as well as where could be customizable.
P.S In fact, I do not really like how the getAssociation function and the Associations class as a whole is done.
He is not very flexible. In fact, it is entirely created for the components of "copies of com_content"
Any difference, for example, the absence of categories or the alias field, or when some other parameters are needed to form associations, leads to usage difficulties.
$advClause in getAssociation function has added a bit of flexibility, but this is just a small patch in a hurry.
Ideally, it is necessary to seriously revise the Multilingual Associations, but this requires a lot of time and effort. And now there are more important tasks
@mbabker @brianteeman @infograf768 @Quy @ggppdk @Arkadiy-Sedelnikov @tonypartridge @CB9TOIIIA @pavluk @ReLater And those who took part in other related PR
What do you think about this PR?